Repository navigation
Stop owned processes when their coroutine exits - #728
Conversation
Track native process handles weakly in coroutine-local state and reuse pool cleanup at owner exit. Preserve detached, fake and non-coroutine starts. Share the output guard through Support while retaining interrupted-stop behavior. Verified ProcessTest, ProcessNonCoroutineTest, real concurrency consumers, source/type analysis and facade consistency.
Guard require and remove output callbacks so their exceptions cannot leave Composer changing the project in the background. Both real-child regressions fail before correction; SupportComposerTest passes.
Apply the shared output guard to MySQL, MariaDB and PostgreSQL dump callbacks without changing schema extension points. Real-child regressions fail before correction; MySQL and PostgreSQL schema-state test files pass.
Prevent a caller-supplied output handler from abandoning the current worker process after an exception. The regression fails before correction; QueueListenerTest passes.
Declare the Process component used directly by Foundation console commands and Composer integration, using the framework's existing version constraint. Package metadata parses successfully; no root dependency versions change.
Start the fixture through the Process API while retaining graceful shutdown before log inspection. SwooleMaintenanceTimerWorkerRecycleTest passes.
Use coroutine-owned subprocesses and return ProcessResult from the profiler helper, preserving output, exit-code and report-cleanup assertions. ProfileScriptTest passes.
Retain each child before starting it and stop all started readers if startup, waiting or an assertion fails. FilesystemNonCoroutineTest passes.
Avoid signaling the test runner's process group when the fixture has no process ID. HttpClientStreamingTest passes with its established platform skip.
Keep real workers and supervisor processes inside exception-safe cleanup while preserving mocked behavior and assertions. Both changed Horizon files pass against an isolated temporary Redis instance.
Keep process-group cleanup limited to a fixture with a known process ID. ShutdownOnInterruptListenerTest passes with its established Swoole skips.
Guard nullable process IDs and remove the startup sleep because Symfony obtains the PID during start. CommanderServeTest passes directly and through package:test with one worker.
Explain coroutine-exit cleanup, explicit detachment and raw Symfony ownership. Recommend the framework Process API for fixtures while retaining explicit shutdown where timing or process trees require it. Documentation matches the reviewed source and behavior tests.
Integrate the merged framework and integration cancellation fixes before publishing process ownership and callback cleanup. Preserve cancellation-aware stop, pool cleanup and regression coverage alongside the previously reviewed weak process tracking and shared output guard. The merge needs no conflict resolution. The process change remains 31 files; differences from its reviewed patch only reflect already-landed dependencies, imports and adjacent cancellation changes. Complete Process and non-coroutine tests, targeted source analysis and formatting pass.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
| Layer / File(s) | Summary |
|---|---|
Coroutine-scoped process ownership src/process/src/ProcessState.php, src/process/src/PendingProcess.php, src/process/src/Pool.php, src/process/composer.json, src/process/README.md, src/docs/processes.md, src/docs/porting-from-laravel.md, src/docs/testing.md, tests/Process/* |
ProcessState tracks processes started in a coroutine and stops them when that coroutine exits. Processes with create_new_console enabled are not registered. The documentation and tests describe ownership, cancellation, detachment, and non-coroutine use. |
Output callback failure cleanup src/support/src/Traits/GuardsProcessOutput.php, src/process/src/InvokedProcess.php, src/process/src/PendingProcess.php, src/database/src/Schema/*, src/queue/src/Listener.php, src/support/src/Composer.php, src/foundation/composer.json, src/support/composer.json, tests/Database/*, tests/Queue/QueueListenerTest.php, tests/Support/SupportComposerTest.php |
GuardsProcessOutput moves to Hypervel\Support\Traits and stops processes when output callbacks throw. Schema dumps, queue workers, and Composer package operations use the guard. Tests check exception propagation and process cleanup. |
Subprocess test APIs and cleanup tests/Cache/*, tests/Filesystem/FilesystemNonCoroutineTest.php, tests/Http/HttpClientStreamingTest.php, tests/Integration/Horizon/Feature/*, tests/Server/Listeners/ShutdownOnInterruptListenerTest.php, tests/Testbench/CommanderServeTest.php, tests/Testing/Profile/ProfileScriptTest.php |
Subprocess tests use Hypervel process APIs in selected cases. Other test changes add process cleanup in finally blocks or guard process-group signals when a PID is unavailable. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant Coroutine
participant PendingProcess
participant SymfonyProcess
participant ProcessState
participant InvokedProcessPool
Coroutine->>PendingProcess: start process
PendingProcess->>SymfonyProcess: start
PendingProcess->>ProcessState: register unless detached
Coroutine-->>ProcessState: exit or cancellation
ProcessState->>InvokedProcessPool: stop tracked processes
Suggested labels: bug
Merge Risk: 
Minimal · previous review d4084
-
No actionable merge blocker is established by the supplied evidence. Merge after the planned tests and checks pass.
Pre-merge checks: 
3 · 
2
-
Failed checks: 2 warnings Passed checks: 3 passedFull details: Description check
-
Explanation
The description explains the problem, behavior change, affected areas, and broad test coverage. It does not follow the required template because it omits the contribution type, explicit supporting-evidence details, verification commands and results, and the required submission checklist.
Resolution
Add the required template sections. Select the contribution type, document the regression test and before/after results, list every verification command with its result, and complete the Before submitting checklist.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 38.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 31 functions across 24 files. (7 skipped: 7 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
|
@coderabbitai review |
Action not completed
|
|
@cubic-dev-ai review |
@binaryfire cubic can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 123,074 of the 120,000 allowed lines of code this month. Reviews resume on 10 November 2026 (in 8 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
PR Summary by QodoStop asynchronous processes when their owning coroutine exits
AI Description
Diagram
High-Level Assessment
Files changed (31)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent |
|
@coderabbitai review |
Action performed
|
A callback failure can already carry cancellation in its exception chain. If emergency stop also throws cancellation, checking only the callback exception class replaces that original failure and loses its cleanup context. Use the shared cancellation-precedence helper when selecting the callback or stop failure. Keep the native-only stop catch, bounded retry and callback re-entry guard unchanged. Extend the existing emergency-stop regression with a wrapped callback failure and assert the retained exception identity. The added row fails before the correction; the complete Process test file, targeted analysis of the trait and its consumers, and formatting pass.
Asynchronous processes left behind by their starting coroutine could retain native process resources until shutdown. Output callback failures could also interrupt cleanup in framework callers using Symfony Process directly.
Track asynchronous processes weakly in their owning coroutine and stop any remaining children when that coroutine exits. Explicitly detached processes remain detached, and processes started outside a coroutine retain their existing lifetime.
Share the output-failure cleanup guard with schema commands, queue listeners and Composer operations. Move suitable test fixtures onto the Process API, retain independent cleanup in termination regressions, and document when tests should use the framework API or Symfony Process directly.
Targeted tests cover coroutine exit, cancellation, callback failures, process pools and non-coroutine execution. Source analysis and affected tests pass; the full suite and service matrices run in CI.
Note
Stop owned processes when their coroutine exits
create_new_consoleand starts outside a coroutine are not tracked.GuardsProcessOutputtosupport/src/Traitsand applies it to schema dumps, queue worker listeners, and Composer package operations so a throwing output callback stops the subprocess first.finally.PendingProcess.startregistration and any callers relying on processes outliving their starting coroutine.Macroscope summarized 57e74a4.
Summary by CodeRabbit