fix: Ensure consistent task finalization for queued tasks (TaskError and TaskProcessed events)

[luanfreitasdev]

What:

• Bug Fix
• New Feature

Description:

This pull request updates the task finalization middleware to ensure that tasks are finalized before passing them to the next middleware, and adds a new test to verify that this behavior also applies to queued tasks. The main changes focus on the correct ordering of middleware operations and improved test coverage for queued tasks.

Middleware behavior update:

• Changed the order in FinalizeTaskMiddleware so that $task->finalize() is called before invoking the next middleware, ensuring tasks are finalized prior to further processing.

Testing improvements:

• Added a new test to verify that queued tasks implementing ShouldQueue are finalized before proceeding to the next middleware. This includes defining a QueuedTask class and a ProcessQueuedTask process, then asserting the appropriate event is dispatched.
• Added ShouldQueue import in the test file to support the new queued task test.

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling in task processing with automatic error event dispatch when failures occur, ensuring proper error tracking and reporting.

• Tests

  • Added test coverage for queued task execution scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Wraps FinalizeTaskMiddleware's call to the next middleware and task finalization in a try/catch that emits a TasksError event, calls the task's fail() on exception, and rethrows; adds a queued-task test verifying finalization behavior and Processed event dispatch.

Changes

Cohort / File(s)	Summary
Middleware: error handling around finalization src/Tasks/Middleware/FinalizeTaskMiddleware.php	Wraps invocation of $next($task) and $task->finalize() in a try/catch; on Throwable, collect error meta, read runProcessId from Context, dispatch TasksError event, call $task->fail($e), and rethrow. Adds imports for TasksError, Context, and Throwable.
Tests: queued task finalization tests/Feature/TaskTest.php	Adds ShouldQueue import and a new test scenario: introduces QueuedTask (extends Task implements ShouldQueue) and ProcessQueuedTask (extends Brain\Process with protected $tasks = [QueuedTask::class]), dispatches the process synchronously, and asserts Processed event is dispatched.

Sequence Diagram(s)

sequenceDiagram
    participant M as FinalizeTaskMiddleware
    participant N as Next Middleware
    participant T as Task
    participant E as EventDispatcher

    rect rgb(220,240,220)
    Note over M,T: Normal (success) path
    M->>N: next($task)
    N-->>M: returns
    M->>T: finalize()
    T-->>M: done
    end

    rect rgb(255,230,230)
    Note over M,E: Error path (new)
    M->>N: next($task)
    N--x M: throws Throwable
    M->>E: dispatch TasksError(task class, payload, runProcessId, meta)
    M->>T: fail(Throwable)
    M-->>M: rethrow Throwable
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review focus:
  • src/Tasks/Middleware/FinalizeTaskMiddleware.php — verify correct meta extraction, Context access, event payload, and rethrow semantics.
  • Tests in tests/Feature/TaskTest.php — confirm the new queued-task classes and assertions correctly exercise finalization order and event emission.

Possibly related PRs

• Enhance Error Handling and Event Logging for Process and Task Classes #16 — Adds TasksError emission and a Task::fail implementation; strongly related to the error-handling additions here.
• Refactor event handling with task lifecycle events and finalization #14 — Modifies the same FinalizeTaskMiddleware control flow; may overlap or conflict with the ordering and finalization behavior.

Suggested reviewers

• r2luna

Poem

🐰 I nudge the code and tidy trails,

finalize swift before it sails.
If errors hop and cause a fray,
I shout TasksError and show the way.
🥕🪄

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: ensuring consistent task finalization for queued tasks and event dispatching in the middleware.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/Feature/TaskTest.php (1)

482-504: Queued-task test doesn’t actually assert middleware ordering and assumes sync queue behavior

The new test is valuable to cover queued tasks, but two points are worth considering:

1. Name vs behavior
   The name queued tasks are finalized before going through next middleware suggests you are asserting relative ordering (i.e., finalize() runs before the next middleware), but the body only checks that Processed was dispatched at all. If ordering is the real concern, you may want to:

   • Add a secondary middleware placed after FinalizeTaskMiddleware that asserts Event::assertDispatched(Processed::class) inside its handle(); or
   • Capture a flag set by finalize() and assert that the flag is true when the “next” middleware runs.

   Alternatively, if you only care that queued tasks get finalized somewhere in the flow, renaming the test to match that behavior would reduce confusion.

2. Assumption about queue driver
   ProcessQueuedTask::dispatchSync() internally calls QueuedTask::dispatch($payload) for ShouldQueue tasks. The test relies on QueuedTask being executed synchronously so that Processed is dispatched before the assertion. If the default queue connection in the test environment is changed to an async driver (database/redis/etc.), this assertion may start failing or become flaky.

   To make the test more robust, consider explicitly forcing a sync driver or equivalent within the test (e.g., via configuration) or documenting that the test suite expects a sync queue connection.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 328d809 and 4506f95.

📒 Files selected for processing (2)

• src/Tasks/Middleware/FinalizeTaskMiddleware.php (1 hunks)
• tests/Feature/TaskTest.php (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/Feature/TaskTest.php (3)

src/Task.php (1)

• Task (34-309)

src/Tasks/Middleware/FinalizeTaskMiddleware.php (1)

• handle (11-16)

src/Process.php (2)

• handle (106-135)
• Process (29-259)

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/Tasks/Middleware/FinalizeTaskMiddleware.php (1)

21-38: Error handling pattern looks good; verify that finalization order still matches queued-task requirements

Wrapping $next($task) and $task->finalize() in a try/catch, emitting a TasksError event, calling $task->fail($e), and rethrowing the Throwable is a solid pattern and preserves the “finalize after pipeline completes” behavior on the happy path.

Given the PR description and earlier review discussion, there was a requirement that queued tasks be finalized before handing off to the next middleware (while keeping the original “finalize after run” semantics for non-queued tasks). This middleware currently calls finalize() only after $next($task) for all tasks. If the “queued tasks finalize-before-next” behavior is still desired and not handled elsewhere, you may need to reintroduce a conditional branch (e.g., for ShouldQueue tasks) and merge it with this new error-handling structure.

Please confirm whether that requirement has changed or is now implemented in another layer; if not, the earlier ShouldQueue-scoped approach likely still applies here.

🧹 Nitpick comments (2)

src/Tasks/Middleware/FinalizeTaskMiddleware.php (2)

19-20: Guard against missing or malformed process context when destructuring

[, $runProcessId] = Context::get('process'); assumes that the process context is always set and is an indexed array with at least two elements. If it’s ever unset or changes shape, this can lead to notices/TypeErrors and an undefined $runProcessId in the catch block.

Consider defensively handling the context (and allowing a null runProcessId) instead of hard-destructuring, for example by checking for an array and using a default:

$processContext = Context::get('process');

$runProcessId = is_array($processContext)
    ? ($processContext[1] ?? null)
    : null;

This keeps the event dispatch resilient even if the context is missing or partially populated.

---

25-39: Reconsider excluding the entire error path from code coverage

// @codeCoverageIgnoreStart before the catch means the whole error-handling branch (event dispatch, fail($e), and rethrow) is invisible to coverage. Since this is non-trivial behavior, it’s usually worth exercising it in tests (e.g., by faking an exception from $next or finalize(), and asserting that the error event is emitted and fail() is called) instead of excluding it.

If feasible, prefer a small dedicated test over blanket @codeCoverageIgnore on the catch block so regressions in error reporting don’t slip through unnoticed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 05afc14 and 61b9831.

📒 Files selected for processing (1)

• src/Tasks/Middleware/FinalizeTaskMiddleware.php (1 hunks)