Harden Signal handling and server process shutdown - #471
Conversation
The inherited integer process constants and positional signal tuples were easy to swap and difficult to validate. SignalManager also exposed initialization and handler inspection methods that applications did not need, while a throwing handler could terminate the only watcher for a signal. Replace the old interface with a Laravel-shaped SignalHandler contract that groups signal numbers under clear worker and server-process keys. Resolve and validate the complete handler definition when listening starts, keep the resolved map local, and remove the retained handler registry and split initialization API. Run each handler through the framework safe-call boundary so one failure is reported without skipping lower-priority handlers or preventing the watcher from listening again. Preserve exact waiter ownership, active-handler completion, and partial-creation rollback. Migrate the server-process stop handler and lifecycle listeners to the new contract. Expand coverage for priorities, invalid definitions, process groups, stopped and non-coroutine paths, repeated delivery after failure, cleanup, and real coroutine creation failure. Remove the unused duplicate fixture and its stale coroutine state.
The default Swoole shutdown allowance was hardcoded to three seconds. Applications with long requests, WebSocket drains, or custom server-process cleanup had no environment-level way to give legitimate work more time before forced termination. Read SERVER_MAX_WAIT_TIME from the environment while preserving the existing three-second default. Cast it to an integer at the configuration boundary, and apply the same normalization to SERVER_WORKERS so numeric environment values reach Swoole with their declared types. Add focused configuration coverage for both values. Reuse the existing environment helper for absent values so every environment source and the cached repository are restored through one exception-safe path instead of duplicating cleanup in the view configuration test.
Signal was becoming an application-facing extension point, but its package README was the only user guide and mixed public behavior with worker-lifecycle details. That left the new handler contract, process groups, and native signal ownership without a canonical documentation surface. Add a Laravel-style Signal guide covering handler definitions, worker and server-process groups, configuration, priorities, failure behavior, process-local delivery, and the complete graceful server-process recipe. Explain the important native boundaries: worker SIGTERM ownership, worker SIGINT behavior, SIGCHLD support, and the process-wide conflict with Swoole Process signal callbacks. Add the guide to the documentation index and link Artisan command users to it when they need server-level handling. Reduce the package README to its documentation and upstream links so the guide remains the single source of truth.
The Server Process guide said configured signal handlers were automatic but did not explain the coroutine requirement or the second half of graceful shutdown. It also omitted reload and health behavior that application developers need when treating a custom process as part of the running service. Clarify that only coroutine-enabled server processes use the server-process signal group. Point readers to the complete stop-handler and running-loop recipe, and document that server reloads do not restart custom processes. Describe the current health boundary without inventing a generic subsystem: custom processes have no built-in readiness, heartbeat, or health state, while applications may publish workload-specific state and inspect it through the existing health event. Replace the duplicate README guide with the canonical documentation link and retained upstream reference.
The server-wide shutdown allowance now has an environment setting, but its scope and edge cases need to be clear before applications tune it. The setting governs more than custom processes and a zero value does not mean unlimited time. Document SERVER_MAX_WAIT_TIME beside the other server environment values. Explain the three-second default, when long requests, WebSocket drains, or process cleanup warrant an increase, and how Swoole treats zero for workers and custom server processes. Clarify that reload commands do not restart custom server processes. Link Reverb worker-recycling guidance to the canonical shutdown section so mixed HTTP and WebSocket deployments size the same server-wide allowance instead of relying on an unnamed timeout.
Record the application-facing Signal re-audit after implementation, full validation, self-review, and independent review. Capture the verified handler failure, inherited API design, malformed configuration, and server shutdown configuration findings together with their final ownership boundaries. Preserve the settled design constraints: grouped string process keys, one safe-call boundary per handler, startup-only validation, exact watcher cleanup, ordinary duplicate configuration behavior, Swoole-owned signal ranges, and no registry, facade, retry, health subsystem, or compatibility layer. Document the completed Contracts, Foundation, Server Process, Reverb, and Signal revalidation, regression coverage, performance result, canonical documentation work, and green repository gates. Add the shared contract and server-setting findings to the cross-package index and keep the active routing entry precise for this worktree until the audit branch is integrated.
Rename the Signal and Server Process guides to match their plural titles and the convention used by other countable framework topics. Update navigation, cross-references, and package README documentation URLs so every link uses the new routes. Add the Signals guide to the published documentation registry, which previously omitted the page despite linking it from the documentation index. Keep the registry sorted and aligned with every indexed guide.
Reset the audit routing index after merging the completed Mail records into the Signal branch. Both work units are complete, so future context restoration should not treat the Signal re-audit as active work or require its ledger entries by default.
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR replaces the signal-handler contract, refactors ChangesSignal lifecycle and shutdown
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SignalRegisterListener
participant SignalManager
participant Container
participant SafeCaller
SignalRegisterListener->>SignalManager: listen(process)
SignalManager->>Container: resolve configured handlers
SignalManager->>SignalManager: create signal watcher coroutines
SignalManager->>SafeCaller: execute handler callback
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Greptile SummaryThe PR redesigns Signal as an application-facing grouped-handler API and hardens watcher execution and shutdown behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/signal/src/SignalManager.php | Consolidates startup resolution and validation into listen(), groups handlers by process and signal, isolates invocation failures, and retains deterministic watcher cleanup. |
| src/signal/src/SignalRegisterListener.php | Routes worker and custom-process lifecycle events to the corresponding string process groups. |
| src/contracts/src/Signal/SignalHandler.php | Replaces positional process/signal tuples with named process groups and signal lists. |
| src/server-process/src/Handlers/ProcessStopHandler.php | Migrates graceful custom-process termination to the new server-process signal group. |
| src/foundation/config/server.php | Normalizes worker count and exposes the server-wide graceful-shutdown allowance as integer environment settings. |
| tests/Signal/SignalManagerTest.php | Expands coverage for grouped definitions, validation, priorities, failure isolation, repeated delivery, and watcher ownership. |
| src/boost/docs/signals.md | Documents the public handler API, process groups, registration, delivery lifecycle, shutdown ownership, and native limitations. |
| src/boost/docs/server-processes.md | Documents server-process lifecycle, reload limitations, health integration, and opt-in graceful signal handling. |
Reviews (2): Last reviewed commit: "Make server config test setup explicit" | Re-trigger Greptile
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/signal/src/SignalManager.php (1)
43-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent duplicate listeners for the same process.
A second
listen($process)call creates another native waiter for every signal. The waiters compete, so a signal can be consumed by an earlier watcher and skip the expected handler flow.Track started process groups before resolving handlers. Return for an already started group. Remove the marker if handler resolution or watcher creation fails.
Proposed fix
class SignalManager { + /** `@var` array<string, true> */ + protected array $listening = []; + public function listen(string $process): void { if (! in_array($process, [SignalHandler::WORKER, SignalHandler::SERVER_PROCESS], true)) { // ... } if ($this->stopped || ! Coroutine::inCoroutine()) { return; } - $signalHandlers = $this->resolveHandlers($process); + if (isset($this->listening[$process])) { + return; + } + + $this->listening[$process] = true; $coroutineIds = []; try { + $signalHandlers = $this->resolveHandlers($process); + foreach ($signalHandlers as $signal => $handlers) { // ... } } catch (Throwable $exception) { + unset($this->listening[$process]); + foreach ($coroutineIds as $coroutineId) { // ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/signal/src/SignalManager.php` around lines 43 - 109, Update SignalManager::listen to track which process groups have already started before calling resolveHandlers, returning immediately when the requested process is already marked. Mark the process as started only when beginning listener setup, and remove that marker if handler resolution or any watcher creation fails, while preserving existing coroutine cancellation and exception propagation.
🧹 Nitpick comments (1)
tests/Foundation/FoundationConfigTest.php (1)
124-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the container binding in
serverConfig()explicit.
serverConfig()discards thenew Application(...)result and depends on the constructor registering itself as the global container instance. If that self-registration changes,server.phpresolves against the previous container and the assertions become misleading. Bind the instance explicitly, astestViewCompiledPathFallsBackToStoragePathWhenDirectoryDoesNotExistdoes at Line 96.♻️ Proposed change
try { - new Application(dirname(__DIR__, 2)); + Container::setInstance(new Application(dirname(__DIR__, 2))); return require dirname(__DIR__, 2) . '/src/foundation/config/server.php'; } finally { Container::setInstance($originalContainer); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Foundation/FoundationConfigTest.php` around lines 124 - 135, Update serverConfig() to store the new Application instance and explicitly set it as the global Container instance before requiring server.php, matching the binding pattern used by testViewCompiledPathFallsBackToStoragePathWhenDirectoryDoesNotExist. Preserve the existing original-container restoration in the finally block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/signal/src/SignalManager.php`:
- Around line 43-109: Update SignalManager::listen to track which process groups
have already started before calling resolveHandlers, returning immediately when
the requested process is already marked. Mark the process as started only when
beginning listener setup, and remove that marker if handler resolution or any
watcher creation fails, while preserving existing coroutine cancellation and
exception propagation.
---
Nitpick comments:
In `@tests/Foundation/FoundationConfigTest.php`:
- Around line 124-135: Update serverConfig() to store the new Application
instance and explicitly set it as the global Container instance before requiring
server.php, matching the binding pattern used by
testViewCompiledPathFallsBackToStoragePathWhenDirectoryDoesNotExist. Preserve
the existing original-container restoration in the finally block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a3b7dd6-2ea2-45a2-b486-a977b305b78c
📒 Files selected for processing (25)
docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mdsrc/boost/docs-ported.mdsrc/boost/docs/artisan.mdsrc/boost/docs/deployment.mdsrc/boost/docs/documentation.mdsrc/boost/docs/reverb.mdsrc/boost/docs/server-processes.mdsrc/boost/docs/signals.mdsrc/contracts/src/Signal/SignalHandler.phpsrc/foundation/config/server.phpsrc/foundation/config/signal.phpsrc/server-process/README.mdsrc/server-process/src/Handlers/ProcessStopHandler.phpsrc/signal/README.mdsrc/signal/src/SignalManager.phpsrc/signal/src/SignalRegisterListener.phptests/Foundation/FoundationConfigTest.phptests/ServerProcess/ProcessStopHandlerTest.phptests/Signal/Fixtures/SignalHandler2Stub.phptests/Signal/Fixtures/SignalHandlerStub.phptests/Signal/SignalManagerCreateFailureTest.phptests/Signal/SignalManagerNonCoroutineTest.phptests/Signal/SignalManagerTest.phptests/Signal/SignalRegisterListenerTest.php
💤 Files with no reviewable changes (1)
- tests/Signal/Fixtures/SignalHandler2Stub.php
Set the temporary Application through the global container before loading the server configuration. Application construction already performs the same registration, but spelling out the dependency keeps this load-bearing setup from looking like an unused constructor call. Keep the existing exception-safe restoration of the previous container and leave runtime configuration behavior unchanged.
|
@coderabbitai I checked both findings against the current source and lifecycle.
The proposed started marker would add worker-lifetime state for an unsupported call and could become false state: a native wait may return The changed Foundation test passes and the final diff is clean. |
|
Tip For best results, initiate chat on the files or code changes.
The Foundation configuration test change addresses the accepted finding. The explicit I withdraw the duplicate Signal listener finding. The supported lifecycle calls ✏️ Learnings added
You are interacting with an AI system. |
Summary
This PR makes the Signal package a reliable application-facing API and documents how it fits into Hypervel's process lifecycle.
It replaces the old tuple-based handler contract, prevents one failing handler from destroying a signal watcher, validates handler configuration before watchers start, and makes the server-wide graceful shutdown allowance configurable. It also adds complete guides for signals and custom server processes.
Problem
Signal handlers previously used integer process constants and positional tuples. The shape was easy to misorder and difficult for static analysis to verify. The manager also exposed initialization and handler inspection methods that applications did not need.
More importantly, an exception from one application handler escaped the watcher coroutine. This skipped later handlers, stopped future deliveries from being watched, and left the next matching signal to the operating system's default behavior.
Custom server processes also had incomplete public guidance around registration, lifecycle, health, reload behavior, IPC, signal ownership, and graceful shutdown. Applications could not increase Swoole's hardcoded graceful shutdown allowance through an environment variable.
Changes
Documentation
The new Signals guide covers handler definitions, process groups, registration, priorities, delivery behavior, worker signal ownership, graceful custom-process shutdown, and native Swoole limitations.
The Server Processes guide covers process definitions, configuration, boot-time registration, lifecycle events, reload behavior, health checks, IPC, and its relationship to the Process facade and Signal package.
Related Artisan, Deployment, Reverb, navigation, and package README links are updated. The public guide slugs use the plural Signals and Server Processes names, matching the rest of the documentation.
Compatibility and performance
Signal has no Laravel counterpart. This intentionally replaces the earlier Hyperf-shaped Hypervel API without a compatibility wrapper. Applications implementing the old contract must move their definitions to the grouped SignalHandler shape.
There is no request-path work. Configuration resolution and validation happen when a worker or custom process starts. Signal delivery adds one safe-call boundary per configured handler. The design does not add locks, retries, registries, polling, or request-scoped state.
Validation
The changed Signal, Server Process, and Foundation configuration tests pass. The complete formatter, static analysis, parallel component, and Testbench gates pass. The Signal package manifest, documentation registry, stale-reference searches, and whitespace checks are clean.
Summary by CodeRabbit
New Features
Bug Fixes
Tests