fix: harden WebSocket transport and watcher process lifecycles#451
Conversation
Use key-existence checks so explicitly stored null values remain distinguishable from missing connection context. Apply dot-aware deletion and filtered copying consistently across the public context API while preserving sibling values and the established missing-source no-op. Add focused coverage for nulls, dotted keys, filtered copies, and get-or-set behavior.
Store each connection's handler class directly under its file descriptor instead of allocating an object that repeats the map key. Update the collector's types and public descriptions to match the simpler representation, and remove the superseded Fd value object. This reduces retained memory per active connection without adding lookup or lifecycle machinery.
Keep handshake state unpublished until the prepared 101 response is emitted and the native session is still established. Resolve all prerequisites before emission, publish without yielding after the liveness check, and release collector and context state on every rejected, failed, canceled, or disconnect-raced exit. Contain failures and cancellation at unguarded message, close, and deferred-open Swoole callbacks so they cannot terminate the worker. Report ordinary callback failures through the application exception handler with contained logger fallbacks, preserve guarded handshake cancellation, and make close cleanup unconditional. Add deterministic coverage for transactional publication, emission failure, rejection, cancellation, disconnect races, independent event and handler failures, reporter fallbacks, cancellation containment, and cleanup without collector metadata.
Return native local push and disconnect results verbatim, distinguish a missing local connection from a failed native call, and fan out only when BASE mode requires cross-worker discovery. Derive worker state from the native server, require at least one recipient and acceptance from every pipe send, cache the server after first resolution, and remove unreachable worker-ID initialization plus per-send logging. Filter unrelated pipe messages before resolving the listener and cover local, PROCESS, BASE, partial-acceptance, frame, cache, and listener paths.
Have the base WebSocket server compile its overridable router so Reverb can retain its isolated router without duplicating the complete bootstrap method. Remove Reverb's superseded bootstrap override and the base server's unread kernel property. Add focused coverage proving that bootstrap resolves the HTTP kernel while compiling the router supplied by the extension hook.
Stop echoing the client's complete Sec-WebSocket-Protocol offer into the handshake response. RFC 6455 permits at most one protocol selected by the application, so middleware may now set an explicit response header while the default handshake selects none. Classify a missing route handler as a server configuration failure instead of a client handshake error, with focused coverage for both behaviors.
Honor every HttpExceptionInterface implementation by retaining its status and headers during failed handshakes. Expose descriptive messages only for client errors, use standard status text for empty or server-side failures, and fall back to a generic internal-server-error response when necessary. Add coverage for client details, empty messages, server redaction, headers, and non-HTTP failures.
Correct the misspelled PATTERN constant and normalize the WebSocket protocol header name inherited from the historical Hyperf implementation. Use the corrected identifiers throughout key validation and add focused coverage for the RFC key example, invalid input, generated handshake headers, and canonical public constant values. No compatibility alias is retained because Hypervel does not require Hyperf API parity.
Declare the native Swoole extension and Symfony HTTP components used directly by the standalone WebSocket Server package, while retaining its complete Hypervel dependency set. Add a split-manifest regression that derives direct Hypervel dependencies from source imports and pins the required extension and Symfony packages so subtree installations cannot rely accidentally on monorepo-only transitive dependencies.
Document the package's Hyperf provenance and add a concise public guide for defining handlers, registering routes, storing connection context, sending locally and across workers, selecting subprotocols, listening for lifecycle events, and using Reverb for application broadcasting. Link the new WebSockets page from the Boost documentation index while keeping transport internals limited to the details users need to build packages and applications correctly.
Record the verified WebSocket Server findings, accepted lifecycle and API corrections, rejected speculative mechanisms, cross-package revalidation, regression coverage, validation, and final performance and complexity assessment. Close the carried Coroutine and HTTP Server revalidation requirements, mark the package complete, and advance the compact audit routing index to Watcher while preserving exact checklist parity with the repository package set.
Centralize absolute target resolution so dot, slash-only, root, and trailing-slash entries behave consistently for every driver. Keep absent roots for long-lived fswatch processes, add polling-oriented existence filtering for sibling drivers, and convert native Swoole exec failure into a named runtime exception without retaining a non-Swoole fallback. Cover root directories, root-level globs, trailing slashes, and fswatch command targets with focused regressions.
Detect GNU find positively and fall back to portable whole-minute intervals when fractional mmin support is unavailable. Process valid stdout even when a scan exits non-zero, report the first failing exit code once per tick, filter missing targets on every poll, include starting-second changes, and tolerate files disappearing before metadata inspection. Run one escaped multi-target Find Newer command per group and advance its reference cutoff only after a complete successful scan, with deterministic coverage for partial failures, late-created targets, and cutoff preservation.
Emit additions, deletions, and modifications independently so one deleted file no longer suppresses valid modification events from the same scan. Handle directory disappearance at the individual root boundary while continuing through later configured roots, and remove the obsolete manual-restart warning now that deletions trigger normal watcher restarts. Add regressions for mixed change batches and missing-root isolation.
Close the result channel when the driver terminates so the coordinator can wait indefinitely without polling for driver completion. Retain the existing one-millisecond debounce only after a change arrives and track pending restart demand as a boolean instead of retaining every changed filename. Preserve driver-failure priority, final-tail restarts, exhaustive cleanup, the outer channel close, and the bounded driver join while removing idle wakeups and burst-dependent memory growth.
Replace queued token-based launches and mutable PID-file reconstruction with one owner coroutine that retains the exact direct-child PID until proc_close reaps it. Coalesce restart demand, honor restart and stop before PID publication, make final stop prevent relaunch, clear ownership after every spawn or cleanup failure, and unpublish the PID immediately after reap before any yield-capable output. Reload the environment at the existing pre-spawn boundary, require explicit non-daemon server configuration, and cover exact signaling, cutoff state transitions, failure recovery, and the post-reap PID invariant deterministically.
Delete the unconsumed Hyperf-derived restart event and dotenv listener now that environment freshness is owned directly by each process-creation boundary. Remove the provider registration with the dead observer surface while preserving the public restart-strategy extension point used by Watcher and Horizon. This leaves one authoritative lifecycle path instead of retaining a compatibility layer with no framework or application consumers.
Make DotenvManager reload remove previously loaded keys, flush the shared repository, clear its one-shot cache, and safe-load the configured file unconditionally. Treat a deleted environment file as authoritative empty state instead of retaining stale worker values, while leaving malformed files visible after prior state is cleared. Route Foundation reloads through the corrected shared boundary and add real missing-file and malformed-file regressions.
Resolve the configured application environment path and reload it immediately before every Horizon child process is created. Ensure initial starts and watcher-driven restarts inherit current values while preserving malformed-file failures and the existing process API. Use the typed Watcher configuration boundary without erasing Horizon's legitimate nullable watch-path fallback, and prove fresh values at both child-creation boundaries in an isolated process test.
Read the package's watcher configuration through the typed repository API without a call-site fallback. The provider-owned configuration file is the single source of defaults, so a missing merged key now fails descriptively instead of silently constructing a divergent empty configuration.
Complete the Watcher split manifest with its direct Swoole, coroutine, Foundation, Console, and Finder runtime dependencies, and remove the coordinator dependency eliminated by blocking channel ownership. Complete Horizon's touched split boundary with the JSON, process-control, Foundation, and Console dependencies used directly by its source. Add package metadata regressions that verify every required split dependency and the removed Watcher coordinator entry.
Record the Watcher package's Hyperf source reference and describe the observable change types produced by each driver. Clarify that the built-in restart strategy owns its direct Swoole server child instead of reconstructing ownership from a PID file, keeping configuration comments and package documentation aligned with the final design.
Record the settled Watcher findings, approved improvements, rejected machinery, implemented ownership model, cross-package dotenv correction, regression coverage, validation, review outcome, and final performance assessment. Mark Watcher complete, close its Support/Foundation/Horizon revalidation, and route the audit to Database with every prior ledger entry that changes its assumptions.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesFramework lifecycle updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebSocketServer
participant Router
participant Handler
Client->>WebSocketServer: send WebSocket handshake
WebSocketServer->>Router: dispatch request
Router-->>WebSocketServer: return handshake response
WebSocketServer->>Client: emit 101 response
WebSocketServer->>Handler: invoke connection callback
Handler-->>WebSocketServer: complete callback
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 SummaryThis PR hardens two long-running runtime boundaries — WebSocket transport and watcher-managed child processes — by making resource ownership explicit and keeping state unpublished until operations have actually succeeded.
Confidence Score: 5/5Safe to merge — well-reasoned hardening of two isolated runtime subsystems with no shared hot-path regressions. The handshake transaction logic correctly handles every rejection path. The Sender send-path now correctly distinguishes missing-fd from send-failure, removing the old bug where both triggered cross-worker delivery regardless of mode. The ServerRestartStrategy lifecycle coroutine correctly gates PID publication and resets all state in its finally block. Watcher driver changes are confined to development-time process management. Test coverage across changed surfaces is thorough. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix(watcher): keep final process stop te..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/support/src/DotenvManager.php`:
- Around line 68-75: Update the reload flow in DotenvManager around
Env::flushRepository() and safeLoad() so registered custom environment adapters
survive repository rebuilding. Preserve or re-register the existing adapters
before safeLoad() loads the updated values, ensuring adapters receive values
such as TEST_VERSION=2.0 while retaining the current cached-value cleanup.
In `@src/watcher/src/Watcher.php`:
- Around line 89-91: Guard the post-loop restart in Watcher::run() so
$this->strategy?->restart() executes only while the watcher lifecycle is still
active, not after SIGINT/SIGTERM teardown has begun. Track or check the existing
stopping/shutdown state set by driver->stop() or strategy->stop(), while
preserving restart behavior for normal restart-pending exits.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab32acac-d3af-4f3a-9713-94c9cf4770d2
📒 Files selected for processing (65)
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/documentation.mdsrc/boost/docs/websockets.mdsrc/foundation/config/server.phpsrc/foundation/src/Listeners/ReloadDotenvAndConfig.phpsrc/horizon/composer.jsonsrc/horizon/src/Console/HorizonRestartStrategy.phpsrc/horizon/src/Console/ListenCommand.phpsrc/reverb/src/Servers/Hypervel/WebSocketServer.phpsrc/support/src/DotenvManager.phpsrc/watcher/README.mdsrc/watcher/composer.jsonsrc/watcher/config/watcher.phpsrc/watcher/src/Console/WatchCommand.phpsrc/watcher/src/Driver/AbstractDriver.phpsrc/watcher/src/Driver/FindDriver.phpsrc/watcher/src/Driver/FindNewerDriver.phpsrc/watcher/src/Driver/FswatchDriver.phpsrc/watcher/src/Driver/ScanFileDriver.phpsrc/watcher/src/Events/BeforeServerRestart.phpsrc/watcher/src/Listeners/ReloadDotenvListener.phpsrc/watcher/src/ServerRestartStrategy.phpsrc/watcher/src/WatchPath.phpsrc/watcher/src/Watcher.phpsrc/watcher/src/WatcherServiceProvider.phpsrc/websocket-server/README.mdsrc/websocket-server/composer.jsonsrc/websocket-server/src/Collector/Fd.phpsrc/websocket-server/src/Collector/FdCollector.phpsrc/websocket-server/src/Context.phpsrc/websocket-server/src/Exceptions/Handler/WebSocketExceptionHandler.phpsrc/websocket-server/src/HandshakeHandler.phpsrc/websocket-server/src/Listeners/InitSenderListener.phpsrc/websocket-server/src/Listeners/OnPipeMessageListener.phpsrc/websocket-server/src/Security.phpsrc/websocket-server/src/Sender.phpsrc/websocket-server/src/Server.phpsrc/websocket-server/src/WebSocketServerServiceProvider.phptests/Foundation/FoundationConfigTest.phptests/Foundation/Listeners/ReloadDotenvAndConfigTest.phptests/Horizon/Console/HorizonRestartStrategyTest.phptests/Horizon/PackageMetadataTest.phptests/Support/DotenvManagerTest.phptests/Support/envs/malformed/.envtests/Watcher/Driver/FindDriverTest.phptests/Watcher/Driver/FindNewerDriverTest.phptests/Watcher/Driver/FswatchDriverTest.phptests/Watcher/Driver/ScanFileDriverTest.phptests/Watcher/Fixtures/ContainerStub.phptests/Watcher/Fixtures/FindNewerDriverStub.phptests/Watcher/PackageMetadataTest.phptests/Watcher/ServerRestartStrategyTest.phptests/Watcher/WatchPathTest.phptests/WebSocketServer/ContextTest.phptests/WebSocketServer/Fixtures/WebSocketMessageStub.phptests/WebSocketServer/HandshakeHandlerTest.phptests/WebSocketServer/PackageMetadataTest.phptests/WebSocketServer/SecurityTest.phptests/WebSocketServer/SenderTest.phptests/WebSocketServer/ServerBootstrapTest.phptests/WebSocketServer/ServerHandshakeTest.phptests/WebSocketServer/ServerTest.phptests/WebSocketServer/WebSocketExceptionHandlerTest.phptests/WebSocketServer/WebSocketServerServiceProviderTest.php
💤 Files with no reviewable changes (7)
- src/watcher/src/Events/BeforeServerRestart.php
- src/websocket-server/src/Collector/Fd.php
- src/websocket-server/src/Listeners/InitSenderListener.php
- src/foundation/src/Listeners/ReloadDotenvAndConfig.php
- src/watcher/src/Listeners/ReloadDotenvListener.php
- src/watcher/src/WatcherServiceProvider.php
- src/reverb/src/Servers/Hypervel/WebSocketServer.php
Reject restart demand while the server restart strategy is in its explicit stopping state. This prevents a late watcher notification from delegating to start() after the owner coroutine has reaped the child and cleared lifecycle ownership. Preserve recovery after natural exits and launch failures, and retain explicit start() as the deliberate way to begin a new lifecycle after final stop. Add a deterministic stop, reap, late-restart regression that also proves an explicit start still works. Update the audit ledger to record the terminal-stop invariant and its regression coverage.
|
@greptileai @coderabbitai Follow-up complete. One finding produced a code change: final stop is now terminal at The other findings were checked against the implementation and left unchanged:
Formatting, both static-analysis configurations, the full parallel suite, and both Testbench suites are green. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Changed paths relevant to ServerRestartStrategy =='
git diff --name-status HEAD^ HEAD -- src/watcher/src/ServerRestartStrategy.php tests/Watcher/ServerRestartStrategyTest.php || true
printf '%s\n' '== Implementation outline =='
ast-grep outline src/watcher/src/ServerRestartStrategy.php --items all --view expanded
printf '%s\n' '== Test outline =='
ast-grep outline tests/Watcher/ServerRestartStrategyTest.php --items all --view expanded
printf '%s\n' '== Lifecycle members and assertions =='
rg -n -C 5 --glob '*.php' \
'(function\s+(start|restart|stop|launchServer|runServer)\s*\(|\$(lifecycleRunning|restartRequested|stopping|processId)\b|terminal|late restart|signal.*stop|reap|explicit.*start)' \
src/watcher/src/ServerRestartStrategy.php tests/Watcher/ServerRestartStrategyTest.php
printf '%s\n' '== Current patch for lifecycle files =='
git diff HEAD^ HEAD -- src/watcher/src/ServerRestartStrategy.php tests/Watcher/ServerRestartStrategyTest.php || trueLength of output: 13375
The explanations for the remaining unchanged findings are accepted as intentional behavior backed by the stated coverage. No further follow-up needed. 🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
All five threads are resolved. The explanations for the four unchanged items are technically sound:
The post-loop restart race was a real issue and c60488e addresses it correctly at the right layer — making This looks good to merge. |
Summary
This PR hardens two long-running runtime boundaries: WebSocket transport and watcher-managed child processes.
The common problem was ownership. WebSocket connections could become visible before the handshake had fully committed, callback failures could escape native Swoole boundaries, and send results did not always describe what the transport accepted. The watcher had the inverse problem: process identity was reconstructed from mutable state while restart requests accumulated around it, and polling drivers could lose changes or retain stale environment state.
The result makes each resource owner explicit and keeps state unpublished until its operation has actually succeeded.
WebSocket transport
101response succeeds and Swoole still reports the session as established.nullvalues.Watcher and managed processes
fswatchdrivers.findpositively, fall back to portable integer-minute behavior, preserve valid output from failed scans, and report failed scan ticks..envnow publishes an authoritative empty environment instead of retaining inherited values.Compatibility and performance
Laravel-facing APIs and configuration remain intact. WebSocket Server and Watcher are Hypervel's low-level Swoole infrastructure, so the removed surfaces are redundant or unused Hyperf-derived internals rather than Laravel APIs.
The normal WebSocket send path is leaner: it avoids repeated container resolution, message formatting, debug logging, and wrapper allocation. Successful handshakes add one native liveness lookup required to close the disconnect race.
Watcher changes are confined to development-time process management. They remove idle polling, bound retained restart state, and launch fewer subprocesses. No request hot path gains a lock, retry loop, registry, acknowledgment protocol, or unbounded state.
Validation
composer fixgate, including formatting, static analysis, the parallel component suite, and both Testbench suites.Summary by CodeRabbit