Skip to content

fix: harden WebSocket transport and watcher process lifecycles#451

Merged
binaryfire merged 24 commits into
0.4from
audit/websocket-transport-watcher-process-lifecycles
Jul 23, 2026
Merged

fix: harden WebSocket transport and watcher process lifecycles#451
binaryfire merged 24 commits into
0.4from
audit/websocket-transport-watcher-process-lifecycles

Conversation

@binaryfire

@binaryfire binaryfire commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

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

  • Make handshake publication transactional. Connection metadata is published only after the prepared 101 response succeeds and Swoole still reports the session as established.
  • Roll back collector and context state on every rejected, failed, canceled, or disconnected handshake path.
  • Contain open, message, and close callback failures at the native callback boundary while preserving application exception reporting and a no-throw logging fallback.
  • Make local and cross-worker sends report the transport result truthfully. Local native failures remain failures, PROCESS mode treats an absent shared connection as authoritative, and BASE mode reports pipe-message acceptance across workers.
  • Preserve frame and binary payload semantics through local and pipe-message paths.
  • Stop implicitly echoing the client's offered subprotocol list. Applications now select one supported protocol explicitly through the response.
  • Preserve HTTP exception status codes and headers during failed handshakes while keeping internal server-error messages private.
  • Make context deletion and filtered copying consistent with dotted keys and stored null values.
  • Remove the redundant per-connection wrapper, unreachable sender initialization, send-path formatting and logging, and duplicated Reverb bootstrap logic.
  • Complete split-package dependencies, provenance, metadata coverage, and task-first WebSocket documentation.

Watcher and managed processes

  • Normalize watched roots and patterns consistently across polling and fswatch drivers.
  • Detect GNU find positively, fall back to portable integer-minute behavior, preserve valid output from failed scans, and report failed scan ticks.
  • Handle missing and late-created roots, changes in the starting second, disappearing file metadata, and simultaneous additions, modifications, and deletions.
  • Run one escaped multi-target command per Find Newer group and advance the reference cutoff only after the complete scan succeeds.
  • Replace the idle millisecond poll with a blocking channel wait and retain only a boolean restart demand during change bursts.
  • Give one coroutine ownership of the watched child process. It retains the exact direct-child PID through reap, coalesces restart demand, handles restart or stop before PID publication, and cannot relaunch after final stop.
  • Reload environment state immediately before every watched child and Horizon child spawn. Removing .env now publishes an authoritative empty environment instead of retaining inherited values.
  • Remove the unused restart event and listener layer, while preserving the public restart strategy extension point.
  • Make non-daemon server mode explicit in the merged framework configuration and complete Watcher and Horizon split metadata.

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

  • Ran the complete composer fix gate, including formatting, static analysis, the parallel component suite, and both Testbench suites.
  • Revalidated focused WebSocket Server, Watcher, Support, Foundation, Horizon, and Reverb coverage.
  • Revalidated real Reverb BASE and PROCESS multi-worker and cross-worker delivery.
  • Validated changed split-package manifests and checked for stale references, package-list drift, and whitespace errors.

Summary by CodeRabbit

  • New Features
    • Added WebSockets documentation (sidebar entry plus a full WebSocket guide: handlers, connection context, sending, and subprotocols).
    • Improved WebSocket handshake, connection publication, messaging, and callback shutdown behavior.
  • Bug Fixes
    • Environment reload now reliably clears previously loaded values when the environment file is missing or invalid.
    • WebSocket error responses now preserve the correct status, headers, and client-facing messaging.
    • Watcher restart and process lifecycle are more deterministic, including better handling of missing/changed targets.
  • Behavior Changes
    • Server startup no longer daemonizes by default.

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.
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 981ac536-e690-44de-94f1-7453885da51a

📥 Commits

Reviewing files that changed from the base of the PR and between 8a5f8c6 and c60488e.

📒 Files selected for processing (3)
  • docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md
  • src/watcher/src/ServerRestartStrategy.php
  • tests/Watcher/ServerRestartStrategyTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
  • docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md
  • src/watcher/src/ServerRestartStrategy.php

📝 Walkthrough

Walkthrough

Changes

Framework lifecycle updates

Layer / File(s) Summary
Environment and package wiring
src/foundation/*, src/support/*, src/horizon/*, src/reverb/*, src/watcher/*, src/websocket-server/composer.json
Environment reloads clear missing or malformed state, daemon mode is explicitly disabled, split-package requirements are declared, Horizon reloads dotenv before process creation, and Reverb uses inherited WebSocket bootstrapping.
Watcher scanning and lifecycle
src/watcher/src/Driver/*, src/watcher/src/Watcher.php, src/watcher/src/ServerRestartStrategy.php, tests/Watcher/*
Watcher targets are normalized and filtered, find failures are logged, file changes are emitted independently, and restart handling uses a single coroutine owner with tracked child PIDs.
WebSocket runtime lifecycle
src/websocket-server/src/*, tests/WebSocketServer/*
Handshake publication is transactional, callback failures and cancellation are contained, connection context is cleaned up, sender results reflect native outcomes, and pipe messages are type-filtered.
Documentation and validation
src/boost/docs/*, src/watcher/README.md, src/websocket-server/README.md, docs/plans/*, tests/*
WebSocket usage and lifecycle behavior are documented, audit ledgers are updated, and tests cover environment reloads, watcher scanning/restarts, WebSocket handshakes, callbacks, sender behavior, context storage, and package metadata.

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
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.07% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: hardening WebSocket transport and watcher process lifecycles.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch audit/websocket-transport-watcher-process-lifecycles

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

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown

Greptile Summary

This 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.

  • WebSocket: Handshake is now transactional (committed flag + finally rollback); native callback failures are contained with a three-layer fallback (ExceptionHandler → logger → error_log); Sender.proxy() distinguishes missing-fd (null) from send-failure (false), and cross-worker delivery is correctly gated on SWOOLE_BASE mode.
  • Watcher: ServerRestartStrategy gives one coroutine ownership of the child process, retaining the exact PID through proc_close, coalescing restart demand, and reloading the environment before every spawn; ScanFileDriver now pushes deleted files to the channel; DotenvManager.reload() clears previously loaded vars so a missing .env publishes an authoritative empty environment.

Confidence Score: 5/5

Safe 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

Filename Overview
src/websocket-server/src/Server.php Handshake made transactional with committed/rollback pattern; callback failures contained at the native boundary with layered fallback reporting; onClose cleanup moved to finally block.
src/websocket-server/src/Sender.php proxy() return type changed to ?bool to distinguish missing-fd (null) from send-failed (false); sendPipeMessage now correctly returns bool, mode check gates cross-worker delivery.
src/watcher/src/ServerRestartStrategy.php One coroutine owns the child process lifecycle; PID retained through reap; stop/restart demand handled correctly before and after PID publication.
src/support/src/DotenvManager.php reload() now deletes previously loaded env vars before flushing the repository so a missing file publishes an authoritative empty environment.

Reviews (2): Last reviewed commit: "fix(watcher): keep final process stop te..." | Re-trigger Greptile

Comment thread src/horizon/src/Console/ListenCommand.php
Comment thread src/watcher/src/Driver/FindDriver.php
Comment thread src/watcher/src/Watcher.php

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ef22b21 and 8a5f8c6.

📒 Files selected for processing (65)
  • docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.md
  • docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md
  • src/boost/docs/documentation.md
  • src/boost/docs/websockets.md
  • src/foundation/config/server.php
  • src/foundation/src/Listeners/ReloadDotenvAndConfig.php
  • src/horizon/composer.json
  • src/horizon/src/Console/HorizonRestartStrategy.php
  • src/horizon/src/Console/ListenCommand.php
  • src/reverb/src/Servers/Hypervel/WebSocketServer.php
  • src/support/src/DotenvManager.php
  • src/watcher/README.md
  • src/watcher/composer.json
  • src/watcher/config/watcher.php
  • src/watcher/src/Console/WatchCommand.php
  • src/watcher/src/Driver/AbstractDriver.php
  • src/watcher/src/Driver/FindDriver.php
  • src/watcher/src/Driver/FindNewerDriver.php
  • src/watcher/src/Driver/FswatchDriver.php
  • src/watcher/src/Driver/ScanFileDriver.php
  • src/watcher/src/Events/BeforeServerRestart.php
  • src/watcher/src/Listeners/ReloadDotenvListener.php
  • src/watcher/src/ServerRestartStrategy.php
  • src/watcher/src/WatchPath.php
  • src/watcher/src/Watcher.php
  • src/watcher/src/WatcherServiceProvider.php
  • src/websocket-server/README.md
  • src/websocket-server/composer.json
  • src/websocket-server/src/Collector/Fd.php
  • src/websocket-server/src/Collector/FdCollector.php
  • src/websocket-server/src/Context.php
  • src/websocket-server/src/Exceptions/Handler/WebSocketExceptionHandler.php
  • src/websocket-server/src/HandshakeHandler.php
  • src/websocket-server/src/Listeners/InitSenderListener.php
  • src/websocket-server/src/Listeners/OnPipeMessageListener.php
  • src/websocket-server/src/Security.php
  • src/websocket-server/src/Sender.php
  • src/websocket-server/src/Server.php
  • src/websocket-server/src/WebSocketServerServiceProvider.php
  • tests/Foundation/FoundationConfigTest.php
  • tests/Foundation/Listeners/ReloadDotenvAndConfigTest.php
  • tests/Horizon/Console/HorizonRestartStrategyTest.php
  • tests/Horizon/PackageMetadataTest.php
  • tests/Support/DotenvManagerTest.php
  • tests/Support/envs/malformed/.env
  • tests/Watcher/Driver/FindDriverTest.php
  • tests/Watcher/Driver/FindNewerDriverTest.php
  • tests/Watcher/Driver/FswatchDriverTest.php
  • tests/Watcher/Driver/ScanFileDriverTest.php
  • tests/Watcher/Fixtures/ContainerStub.php
  • tests/Watcher/Fixtures/FindNewerDriverStub.php
  • tests/Watcher/PackageMetadataTest.php
  • tests/Watcher/ServerRestartStrategyTest.php
  • tests/Watcher/WatchPathTest.php
  • tests/WebSocketServer/ContextTest.php
  • tests/WebSocketServer/Fixtures/WebSocketMessageStub.php
  • tests/WebSocketServer/HandshakeHandlerTest.php
  • tests/WebSocketServer/PackageMetadataTest.php
  • tests/WebSocketServer/SecurityTest.php
  • tests/WebSocketServer/SenderTest.php
  • tests/WebSocketServer/ServerBootstrapTest.php
  • tests/WebSocketServer/ServerHandshakeTest.php
  • tests/WebSocketServer/ServerTest.php
  • tests/WebSocketServer/WebSocketExceptionHandlerTest.php
  • tests/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

Comment thread src/support/src/DotenvManager.php
Comment thread src/watcher/src/Watcher.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.
@binaryfire

Copy link
Copy Markdown
Collaborator Author

@greptileai @coderabbitai Follow-up complete.

One finding produced a code change: final stop is now terminal at ServerRestartStrategy. A restart arriving after signal-driven stop and reap is ignored until an explicit start(). The regression covers stop, reap, late restart, and verifies that explicit start remains valid.

The other findings were checked against the implementation and left unchanged:

  • nullable presence handling followed by typed config validation intentionally requires the two Horizon config reads;
  • the start-second boundary prefers one possible extra development restart over silently missing a real change;
  • closed Swoole channels preserve buffered entries, and existing tests cover the exact producer-close-drain schedule;
  • custom environment adapter factories survive repository flush and are invoked during rebuild, as the cited regression proves;
  • the docstring threshold is not a repository check, and blanket comments would add noise rather than useful documentation.

Formatting, both static-analysis configurations, the full parallel suite, and both Testbench suites are green.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

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 || true

Length of output: 13375


@binaryfire Verified. restart() now exits when ServerRestartStrategy::$stopping is set, and the new regression covers the complete terminal path: stop/reap, ignored late restart, and successful explicit start() afterward.

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!

@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown

All five threads are resolved. The explanations for the four unchanged items are technically sound:

  • The two-read Horizon pattern is intentional — get() handles the null/fallback path while array() enforces the type, and my suggested form also performs two reads.
  • The < boundary is the correct tradeoff given filemtime()'s one-second resolution — <= would silently miss real changes rather than risk one extra restart.
  • Swoole channels do drain buffered entries after close before returning false, so the old explicit drain loop was redundant, and the cited tests confirm this.
  • flushRepository() clears constructed instances but not the factory registry, so adapters are re-registered on the next getRepository() rebuild.

The post-loop restart race was a real issue and c60488e addresses it correctly at the right layer — making ServerRestartStrategy the authoritative terminal-state owner rather than adding a flag in Watcher.

This looks good to merge.

@binaryfire
binaryfire merged commit 6d0bbd9 into 0.4 Jul 23, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant