Harden lifecycle and concurrency robustness by binaryfire · Pull Request #430 · hypervel/components

binaryfire · 2026-07-12T08:58:04Z

Summary

A rare parallel test-suite hang exposed a broader class of lifecycle bugs: work could reserve capacity or register cleanup state before spawning a coroutine, while native coroutine creation could fail without a consistent framework contract. Similar ownership gaps existed around timers, pooled connections, child processes, file watchers, subscribers, and test teardown.

This PR fixes those problems at their owning boundaries. Coroutine creation now either returns an integer ID or throws a typed exception. Stateful callers roll back transactionally. Long-lived and asynchronous resources have explicit owners, bounded shutdown behavior, and exception-safe cleanup. The test suite exercises those contracts directly instead of relying on timing, retries, fixed ports, or unbounded waits.

The same audit also corrects several adjacent issues: compiled route objects no longer cross collection identity, programmatic console calls no longer inherit process-global shell verbosity, pool discard restores capacity, Swoole table lock acquisition is bounded, and Testbench verifies process incarnation before signaling a stale PID.

For more detailes, see: docs/plans/2026-07-12-test-suite-lifecycle-and-concurrency-robustness.md

Motivation

The captured hang was a CPU-bound queue worker test. Its fake sleep method recorded the delay but did not yield, allowing the daemon loop to starve a child job coroutine indefinitely. Fixing that one fake would address the observed symptom, but tracing the ownership chain found multiple ways the suite or a production worker could still fail to make progress:

Swoole returns false when coroutine creation fails, while framework layers variously expected an integer, -1, or false.
Capacity tokens, wait-group counts, timer registrations, and signal handlers could be committed before a failed spawn.
Cleanup failures could skip later independent cleanup, including worker-exit coordination and framework-static resets.
Queue and Horizon termination paths could wait indefinitely on work that was no longer capable of making progress.
Watcher drivers exposed inconsistent lifetimes: one blocked while others registered detached timers and returned.
Test-owned processes, sockets, subscribers, and IPC channels had paths with unbounded waits or success-only cleanup.
The testing database resolver extracted a bare connection and abandoned the pooled wrapper that owned its capacity.

These are lifecycle contract problems, not timing problems. Adding retries or larger timeouts would make the failures rarer without making ownership correct.

Coroutine creation

Engine coroutine creation now has one contract:

success returns a positive integer coroutine ID;
native creation failure throws CoroutineCreateException.

The high-level Coroutine::create(), Coroutine::fork(), go(), and co() APIs expose the same behavior. Callers that reserve state before spawning now roll it back explicitly, including concurrent work limits, wait groups, timers, signal watchers, Redis subscriber loops, prompt animation state, and Reverb pub/sub startup.

The native HTTP server callback handles coroutine exhaustion at the callback boundary and completes the request with HTTP 503. Database and Redis health checks intentionally translate only coroutine creation failure into an unhealthy result.

Cleanup and test ownership

Coroutine test teardown now runs every independent cleanup action and preserves the first failure. Test-body failures remain primary while context cleanup, native timer cleanup, worker-exit resume, and coordinator cleanup still execute.

Framework base test cases verify Mockery expectations during their own teardown so failures are attributed to the test that created them. The global PHPUnit subscriber remains a fallback for tests using another base class and keeps framework-static cleanup authoritative. Throwable pooled-database cleanup is captured before the pure static reset list, so it cannot skip unrelated resets.

Tests that create processes, sockets, subscribers, or child coroutines now own them through bounded joins and finally cleanup. The cache multiprocess harness uses framed nonblocking IPC, monotonic deadlines, and owned-PID reaping. Engine socket tests use ephemeral ports and explicit readiness instead of fixed ports or synthetic probe connections.

Pools and testing database connections

Connection pools now expose an explicit discard() operation for destroying a borrowed connection while restoring capacity. Connection and object pool channels treat a failed outside-coroutine wake as a missed notification after state has already been committed. Checkout performs exactly one final immediate state pass at its deadline, closing the ordinary timeout-versus-release race without polling or a permanent dispatcher coroutine.

The testing database resolver retains the pooled wrapper alongside its stable bare connection. Reusable reset and terminal flush are separate operations, and teardown discards wrappers before closing the pool. Shared in-memory SQLite keeps its pool-owned PDO while wrapper-owned transactions are still rolled back correctly.

Queue and Horizon process lifecycle

Queue worker timeout monitoring is owned by one injected timer whose exact registration is cleared on every daemon exit. Monitor locks reset through finally, test workers retain scheduler progress without installing process signal handlers, and a hard job timeout terminates the poisoned worker immediately instead of waiting for unrelated coroutines.

Horizon persistence is synchronous with the loop iteration that owns it, so writes cannot overlap or report failures outside that boundary. Worker termination uses the existing graceful state machine and applies configured hard-stop deadlines.

A parent now blocks each child process's control signals across fork and exec. The child unblocks the exact set only after installing its handlers, so an early pause or termination signal remains pending instead of taking its default disposition. The parent restores its prior signal mask on both successful and failed startup. This intentionally does not use Symfony's ignored-signal API because that can suppress later Process::signal() calls on SIGCHLD-enabled builds.

Watcher lifecycle

Every watcher driver now owns one blocking, explicitly stoppable lifecycle. Polling drivers use an owned stop channel rather than detached timers. Fswatch preserves newline framing across partial reads, processes matching inline, and propagates terminal failures through the owned driver coroutine.

Watcher observes driver completion and failure, drains final synchronous batches, stops both the driver and restart strategy, closes the change channel, and performs a bounded join without masking the primary failure. Restart strategies expose an idempotent stop() contract. Server restart handling restores launch capacity in finally and validates positive PID input before dispatching events or sending POSIX signals.

Additional correctness fixes

Compiled named-route objects are cached by collection rather than globally by route name.
Programmatic console execution bypasses Symfony's process-global CLI wrapper while retaining explicit IO option behavior and console event semantics.
Cache and Reverb striped locks retain a one-CAS uncontended path but use bounded acquisition and release partial all-lock ownership on failure.
Reverb Redis startup owns the exact subscriber through handshake, queued publish drain, consumer spawn, disconnect, and reconnect.
Config and route cache subprocesses receive their parent's resolved cache paths.
Testbench process cleanup requires matching PID, command, parent, and process-start identity before signaling a live process.
Empty pool frequency windows return zero instead of dividing by an empty sample.
Testbench file-producing suites restore every owned source and generated file even when an earlier cleanup action fails.

Compatibility

This targets the Hypervel 0.4 architecture and deliberately establishes stricter low-level contracts:

coroutine creation no longer returns false or -1;
connection pools gain explicit discard semantics;
watcher drivers and restart strategies have explicit blocking and stop contracts;
the unused Reverb pub/sub subscribe() contract method is removed;
queue worker test seams are replaced by an owned Timer dependency.

These changes make invalid lifecycle states unrepresentable rather than retaining compatibility shims for ambiguous behavior.

Verification

Ran the full composer fix gate, including formatting, static analysis, the parallel component suite, Testbench, and dogfood.
Ran focused process-isolated coroutine-exhaustion regressions.
Ran focused Redis, Reverb, Horizon, queue, pool, watcher, signal, socket, Mockery, and Testbench lifecycle tests.
Reviewed the complete diff for caller and implementer coverage, stale intermediate designs, exception masking, liveness, coroutine safety, and hot-path overhead.

Summary by CodeRabbit

New Features
- Coroutine creation helpers now return positive IDs and throw on failure.
- Added connection discard operations for pooled connections and direct removal from pools.
- Watchers now expose explicit stop controls; console call() supports consistent programmatic execution.
Bug Fixes
- Improved robustness of task, timer, signal, socket, and pool lifecycles with clearer failure propagation and fewer deadlocks/leaks.
- HTTP now returns a clear 503 Service Unavailable when coroutine capacity is exhausted.
- Route cache behavior is now scoped per collection to avoid cross-contamination.
Documentation
- Updated guidance for async test resource ownership, Mockery teardown/verification, and lifecycle cleanup order.

Document the verified causes of the intermittent parallel-suite hang and the related lifecycle, ownership, and concurrency defects uncovered during the audit. Define the final low-level designs for coroutine creation failures, exhaustive cleanup, pool ownership, queue and Horizon process control, watcher lifecycles, Testbench isolation, and deterministic programmatic console execution. Include the complete implementation order, regression matrix, performance constraints, and overengineering guardrails used to validate the work. Normalize older plan headers by removing obsolete author, date, and status metadata while retaining their scope sections.

Replace the ambiguous false and -1 creation outcomes with a typed CoroutineCreateException at the Engine boundary. Make the high-level create, fork, go, and co APIs return positive integer IDs on success and propagate creation failure consistently. Document the new contract and add process-isolated regressions that exhaust Swoole's coroutine capacity without polluting the surrounding test worker.

Roll back capacity tokens and wait-group counts when a child coroutine cannot be created. Record keyed failures in Parallel and the concurrency driver so their normal aggregation and input-order exception behavior remains intact instead of hanging on bookkeeping for work that never started.

Treat inability to create a bounded database or Redis probe coroutine as an unhealthy pooled connection. Catch only CoroutineCreateException so connection and application failures retain their existing behavior while pool maintenance avoids the obsolete boolean creation contract.

Make SafeSocket roll back its send-loop state when coroutine creation fails, treat native send failures as terminal, preserve native error details, and accept the valid payload string zero. Refactor socket regressions to use ephemeral ports, explicit readiness, parent-visible child failures, bounded joins, and unconditional client and server cleanup so parallel tests cannot collide or leak listeners.

Catch CoroutineCreateException at the native Swoole request callback boundary, where the child coroutine's internal exception handler cannot observe a failed spawn. Log the overload and complete the response with HTTP 503 without invoking the application handler or leaking an exception through native code.

Roll back only the handlers and native waiters created by a failed registration call. Use exception-injecting cancellation so Swoole wait loops terminate, treat intentional cancellation as control flow, and prevent partially installed signal sets from surviving coroutine exhaustion.

Make subscriber construction roll back its receive loop, shutdown timer, connection, and every channel when either background registration fails. Retain the shutdown timer ID for normal interruption and preserve the original construction failure if cleanup also encounters an error.

Move Task and Spinner animation creation inside their existing exception-safe lifecycle. Cursor visibility and final rendering now recover even when Swoole cannot create the animation coroutine, with focused regressions for both prompt implementations.

Remove an unnecessary shutdown-time coroutine and resume the worker-exit coordinator in finally after listener dispatch. This guarantees shutdown waiters are released even when a listener throws and avoids consuming a coroutine slot while the worker is already exiting.

Capture coordinator identity before spawning timer work so teardown cannot make a delayed child resolve a fresh open coordinator. Roll back registrations when creation fails, stop cleared ticks before another interval, and preserve immediate zero-timeout behavior with focused lifecycle regressions.

Run each coroutine teardown hook and every independent framework cleanup action even when an earlier action fails. Preserve the test-body exception or first teardown failure while still clearing context, native timers, and worker-exit coordination so a failed test cannot strand later work.

Move Mockery verification into the shared framework base-test lifecycle so unmet expectations are attributed to the test that created them. Keep the PHPUnit subscriber as an exhaustive fallback, capture throwable resource cleanup separately from the pure static-reset registry, and preserve first-failure ordering. Remove the duplicate Testbench trait, cover all supported base cases and failure combinations, and update repository guidance so individual tests neither close Mockery nor duplicate framework-owned resets.

Add an explicit discard operation to connection pools so borrowed resources can be destroyed while restoring capacity. Enforce managed and borrowed ownership across connection wrappers and preserve structurally aligned connection-pool and object-pool behavior. Make outside-coroutine wake failure non-throwing after state is committed, and give checkout exactly one final idle and capacity pass at its deadline. Cover release, discard, missed wake, foreign ownership, and no-second-wait behavior in both pool implementations.

Keep each testing resolver's owning pooled wrapper alongside its stable bare database connection, then discard the wrapper explicitly during named, terminal, and container-change cleanup. Split reusable reset behavior from terminal flush behavior and order resolver teardown before pool shutdown. Always detach a discarded wrapper from its connection while preserving the pool-retained shared in-memory SQLite PDO, including rollback of transactions owned by that wrapper. Add max-one-capacity, write-routing, shared-database, transaction, and lifecycle regressions.

Replace ad hoc timeout-monitor callables with one injected Timer whose exact registration is cleared on every daemon exit. Reset the monitor lock through finally, keep timeout-job suppression intact, and make the recording test worker yield without installing real signal handlers. Treat a hard job timeout as terminal for the poisoned worker process instead of waiting indefinitely for unrelated job coroutines. Unblock the worker's exact control-signal set only after its handlers are installed so a supervising parent can protect the bootstrap window. Cover monitor failures, every daemon return path, scheduler progress, stopping events, signal ownership, and immediate process termination under concurrency.

Block each child's exact handled signal set across Symfony Process startup and restore the parent's prior mask on success or failure. Unblock pending signals only after queue-worker or supervisor handlers are installed, without using Symfony's ignored-signal API that can suppress later control sends on SIGCHLD builds. Add an immediate hard-stop primitive and regressions for delayed handler installation, pending-signal delivery, mask restoration, parent-child signal-set parity, and zero-grace process termination.

Persist supervisor and master state synchronously before loop events so writes cannot overlap, reorder, or escape the initiating error boundary. Scale workers through the existing terminating state machine, enforce configured grace periods, and hard-stop children that remain alive at their deadline. Replace eventual-persistence retries and permissive teardown with deterministic ordering, exhaustive active and terminating process cleanup, and regressions for persisted state visibility and bounded master shutdown.

Give Cache and Reverb striped locks a fixed internal acquisition deadline with backoff after the existing hot-spin window. Preserve the uncontended path as one compare-and-set and release every stripe acquired before a later all-lock timeout. Move Reverb's full-table logging outside critical sections and cover contention recovery, holder death, partial acquisition, and all failed lock-row call sites without adding user-facing lock policy.

Keep a new subscriber locally owned through creation and subscription, recheck disconnect state after yielding operations, and commit only the exact subscriber consumed by the spawned receive loop. Clear and reconnect by object identity so an older consumer cannot tear down a replacement. Reset retry state only after complete startup, drain queued publishes in order, drop permanently invalid JSON payloads, and retain transient publish failures with their ordered tail. Remove the unused public subscribe method and cover spawn, handshake, disconnect, retry-limit, and queue-drain failures.

Move named Route object caching from process-global static state onto the CompiledRouteCollection that owns the route attributes. Replacing a collection now establishes object identity without requiring a global flush while retaining worker-lifetime reuse for the active collection. Remove obsolete reset and warmup guidance and prove that collections sharing a route name cannot exchange domain, port, URI, action, or object identity.

Route Application::call through a dedicated IO configuration and doRun boundary instead of Symfony's root CLI wrapper. Programmatic commands now ignore inherited SHELL_VERBOSITY, avoid process-global exception and terminal mutation, and still honor explicit ANSI, interaction, quiet, and verbosity options. Add behavioral parity coverage for command execution, help, output, events, exceptions, and explicit options while retaining command cloning as the existing concurrent and nested-call isolation boundary.

Standardize driver watch as a blocking operation that returns only after terminal completion or idempotent stop. Replace detached coordinator timers with one lazily owned stop channel so polling cadence remains coroutine-friendly while stop wakes an interval immediately and scan failures reach the Watcher owner. Preserve FindNewer's reference-file and in-flight scan cleanup rules and cover real interval blocking, immediate stop, deferred cleanup, and propagated scan failures across every polling driver.

Process fswatch paths inline in the owned driver coroutine, retain incomplete newline-delimited tails across reads, and deliver complete paths in order under bounded channel backpressure. Terminal read and matching failures now propagate through the driver's lifecycle instead of detached raw coroutines. Compile immutable WatchPath glob patterns once and add regressions for split paths, multi-record chunks, final buffered records, ordered batches, failure propagation, and explicit process teardown.

Own the blocking driver in one joined coroutine, propagate its terminal failure, drain synchronous final batches, and perform driver, strategy, channel, and bounded-join cleanup without replacing the primary operation error. Add an idempotent restart-strategy stop contract, restore server launch tokens in finally, report asynchronous native launch failures safely, and validate positive PID files before any event or POSIX signal. Cover full-channel shutdown, debounce tails, restart failures, managed-process cleanup, and corrupted PID input.

Give every forked cache-test child an explicit PID owner, nonblocking length-prefixed result channel, monotonic deadline, maximum frame size, and exhaustive failure cleanup. Retry interrupted reaping, accept only the owned PID or ECHILD, and never fall back to a global or blocking wait. Add deterministic early-exit, incomplete-payload, child-error, and stall coverage so process failures report promptly instead of hanging a parallel worker.

Replace an unbounded hand-built renderer coroutine join with the framework parallel primitive so child exceptions propagate through an owned wait group. Wrap explicitly created Redis subscribers and raw publish connections in exhaustive cleanup so assertion failures cannot leave background receive work alive in the test worker.

Introduce a small test-only cleanup primitive that runs every supplied owner callback and rethrows the first failure afterward. This gives file-producing integration tests one deterministic way to preserve primary failures without skipping independent restoration or parent teardown.

Pass each application's resolved config and route cache path into the fresh subprocess that rebuilds it. PHP array-only environment overrides are not inherited automatically, so this prevents a child from reading or writing a stale default cache that differs from its parent. Add alternate-path regressions and make the route-cache suite exhaustively own its generated sources and caches while asserting that worker Testbench state is pristine before each run.

Convert every remaining route- and provider-producing Testbench suite to checked, atomic restoration and deletion through CleanupActions. Keep each test's owned resource list local, preserve the first cleanup failure, and still run every later file repair and parent teardown action to prevent worker contamination.

Explain that tests must close or join child coroutines, subscribers, processes, servers, and similar resources through exception-safe ownership. Recommend the framework parallel primitive over unbounded hand-built channel joins when channel behavior is not under test. Document that Hypervel verifies and closes Mockery automatically through framework base cases with a global subscriber fallback, so application and package tests must not call Mockery::close themselves.

coderabbitai · 2026-07-12T08:58:43Z

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: d3968f5a-be8c-4b79-a3fc-a21a14b93878

📥 Commits

Reviewing files that changed from the base of the PR and between ba8cfee and 88b864e.

📒 Files selected for processing (22)

docs/plans/2026-07-12-test-suite-lifecycle-and-concurrency-robustness.md
src/filesystem/src/Filesystem.php
src/foundation/src/Testing/DatabaseConnectionResolver.php
src/horizon/src/WorkerProcess.php
src/queue/src/Worker.php
src/session/src/FileSessionHandler.php
src/testbench/src/Bootstrapper.php
src/testing/src/PHPUnit/AfterEachTestSubscriber.php
src/watcher/src/Events/BeforeServerRestart.php
src/watcher/src/ServerRestartStrategy.php
tests/Cache/CacheSwooleStoreConcurrencyTest.php
tests/Coordinator/TimerTest.php
tests/Filesystem/FilesystemTest.php
tests/Foundation/Exceptions/Renderer/ListenerContextIsolationTest.php
tests/Foundation/Testing/DatabaseConnectionResolverTest.php
tests/Integration/Horizon/Feature/WorkerProcessTest.php
tests/Queue/QueueWorkerTest.php
tests/Reverb/Servers/Hypervel/Scaling/SwooleTableSharedStateLockTest.php
tests/Session/FileSessionHandlerTest.php
tests/Testbench/BootstrapperTest.php
tests/Testing/PHPUnit/AfterEachTestSubscriberTest.php
tests/Watcher/ServerRestartStrategyTest.php

🚧 Files skipped from review as they are similar to previous changes (13)

src/watcher/src/Events/BeforeServerRestart.php
tests/Foundation/Exceptions/Renderer/ListenerContextIsolationTest.php
tests/Coordinator/TimerTest.php
tests/Reverb/Servers/Hypervel/Scaling/SwooleTableSharedStateLockTest.php
src/testing/src/PHPUnit/AfterEachTestSubscriber.php
src/watcher/src/ServerRestartStrategy.php
docs/plans/2026-07-12-test-suite-lifecycle-and-concurrency-robustness.md
src/foundation/src/Testing/DatabaseConnectionResolver.php
tests/Cache/CacheSwooleStoreConcurrencyTest.php
src/testbench/src/Bootstrapper.php
src/queue/src/Worker.php
tests/Foundation/Testing/DatabaseConnectionResolverTest.php
tests/Queue/QueueWorkerTest.php

📝 Walkthrough

Walkthrough

This PR hardens coroutine creation, cleanup ownership, pool and lock lifecycles, process shutdown, console execution, Redis handling, routing caches, and watcher lifecycles. It adds extensive regression coverage and updates related documentation and fixtures.

Changes

Concurrency and lifecycle robustness

Layer / File(s)	Summary
Coroutine failures and rollback `src/engine/...`, `src/coroutine/...`, `src/concurrency/...`, `src/coordinator/...`	Coroutine creation now throws typed exceptions, while callers restore permits, wait groups, timer registrations, and resource state on failure.
Pools, locks, caches, and cleanup `src/pool/...`, `src/cache/...`, `src/reverb/...`, `src/routing/...`, `src/foundation/src/Testing/...`, `src/testing/...`	Discard operations, bounded lock acquisition, collection-owned route caches, database wrapper cleanup, Mockery ownership, and first-failure cleanup handling are implemented.
Workers, processes, sockets, and watchers `src/queue/...`, `src/horizon/...`, `src/engine/src/SafeSocket.php`, `src/watcher/...`, `src/testbench/...`	Timers, signal masks, process termination, socket state, watcher shutdown, restart strategies, and runtime-process identity checks are made explicit and bounded.
Regression coverage and documentation `tests/...`, `docs/...`, `AGENTS.md`, `src/boost/docs/...`	Tests cover coroutine overload, cleanup, sockets, pools, locks, console parity, signals, Horizon, Redis, Testbench identity, and watcher shutdown behavior; documentation records the updated contracts.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

hypervel/components#401: Related Redis heartbeat and coroutine health-check handling.
hypervel/components#414: Related Swoole table lock and cache race handling.
hypervel/components#415: Related AfterEachTestSubscriber cleanup ordering.

Suggested reviewers: albertcht

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 46.21% 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 matches the PR’s central theme and accurately summarizes the broad lifecycle and concurrency hardening changes.
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 fix/test-suite-lifecycle-robustness

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

Use WorkerOptions as the single source of truth for timeout-monitor cadence so the documented queue:work --monitor-interval option reaches the owned Timer. Remove the redundant Worker constructor interval that was never populated by the service provider and always forced the one-second default. Record the requested timeout in the Timer test seam and prove that a non-default option is used during daemon startup. Update the lifecycle plan with the pre-existing wiring bug, final design, and regression coverage.

coderabbitai

Actionable comments posted: 13

🧹 Nitpick comments (5)

src/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.php (1)
250-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep the new method docblock title-only.

Remove the explanatory paragraph; retain @phpstan-impure if required.
Suggested cleanup
 /**
  * Determine whether reconnect work remains enabled.
- *
- * Hooked Redis I/O and Sleep may yield while disconnect() changes this state.
  *
  * `@phpstan-impure`
  */
As per coding guidelines, “Add title-only Laravel-style method docblocks to methods.”
🤖 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/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.php` around lines
250 - 256, Update the docblock for the reconnect-state method near the Redis
scaling provider to retain only its title and the required `@phpstan-impure`
annotation; remove the explanatory paragraph about Redis I/O, Sleep, and
disconnect().
Source: Coding guidelines
src/routing/src/CompiledRouteCollection.php (1)
235-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep the updated method docblocks title-only.

Remove the new cache-behavior paragraphs while retaining necessary static-analysis annotations.
Suggested cleanup
 /**
  * Get a route instance by its name.
- *
- * Returns cached Route objects for the lifetime of this collection.
  */
@@
 /**
  * Get the route instances that should be pre-warmed.
- *
- * Returns the collection's cached Route instances — these
- * are the objects actually used during request matching. Unlike
- * getRoutes() which creates fresh throwaway objects every call.
  *
  * `@return` array<int, Route>
  */
As per coding guidelines, “Add title-only Laravel-style method docblocks to methods.”
🤖 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/routing/src/CompiledRouteCollection.php` around lines 235 - 288, Update
the docblocks for getByName and getRoutes in CompiledRouteCollection to use
title-only Laravel-style descriptions. Remove the cache-behavior explanatory
paragraphs while retaining required static-analysis annotations such as the
getRoutes return type.
Source: Coding guidelines
src/testing/src/PHPUnit/AfterEachTestSubscriber.php (1)
48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Import the database resolver.

Use a DatabaseConnectionResolver import and its short name for this new call. As per coding guidelines, “Import classes with use statements and reference short names.”
🤖 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/testing/src/PHPUnit/AfterEachTestSubscriber.php` around lines 48 - 50,
Update the try block in AfterEachTestSubscriber to import
Hypervel\Foundation\Testing\DatabaseConnectionResolver with a use statement and
call flushCachedConnections() through the short DatabaseConnectionResolver name
instead of the fully qualified class name.
Source: Coding guidelines
src/foundation/src/Testing/DatabaseConnectionResolver.php (2)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align the new lifecycle docs with the project rules.

resetCachedConnections() and flushCachedConnections() need a Tests only. warning naming the cross-coroutine stale/discarded-connection failure mode. Keep the added flush() and connection() method docblocks title-only. As per coding guidelines, public worker-lifetime mutators require scope warnings and concrete persistence failures, while method docblocks must be title-only.

Also applies to: 88-96, 125-129, 141-145
🤖 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/foundation/src/Testing/DatabaseConnectionResolver.php` at line 58, Update
the docblocks for resetCachedConnections() and flushCachedConnections() to
include a “Tests only.” warning describing the cross-coroutine
stale/discarded-connection failure mode. Keep the flush() and connection()
method docblocks title-only, without adding explanatory text or warnings.
Source: Coding guidelines

39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Expose standard cleanup for the new static wrapper cache.

Add a flushState() method delegating to terminal wrapper cleanup, and have AfterEachTestSubscriber call it. This makes the new process-global cache follow the framework static-state lifecycle contract. As per coding guidelines, “When static state is introduced or modified, ... add flushState() for framework static properties and register cleanup with AfterEachTestSubscriber when required.”
🤖 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/foundation/src/Testing/DatabaseConnectionResolver.php` around lines 39 -
44, Add a public static flushState() method to DatabaseConnectionResolver that
delegates cleanup to the terminal wrapper mechanism, ensuring the
pooledConnections cache is cleared. Update AfterEachTestSubscriber to invoke
DatabaseConnectionResolver::flushState() during per-test cleanup, following the
framework static-state lifecycle.
Source: Coding guidelines

🤖 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/testbench/src/Bootstrapper.php`:
- Around line 168-179: Make the post-copy initialization around Bootstrapper’s
$filesystem->replace() exception-safe: ensure the copied runtime directory is
removed if marker creation or related setup throws, then rethrow the original
exception. Preserve normal $runtimePath recording and shutdown cleanup
registration when initialization succeeds.

In `@src/testing/src/PHPUnit/AfterEachTestSubscriber.php`:
- Around line 54-57: Update the cleanup flow in AfterEachTestSubscriber so
flushFrameworkState() exceptions are caught and assigned to $exception only when
no earlier callback, Mockery, or database cleanup failure exists; then preserve
the existing final rethrow behavior.

In `@src/watcher/src/Driver/DriverInterface.php`:
- Around line 14-15: Update the PHPDoc for the watch() method in DriverInterface
to contain only its title, and move the lifecycle-blocking details to an
interface-level docblock. Preserve the documented behavior while ensuring
watch() uses title-only Laravel-style PHPDoc.

In `@src/watcher/src/Events/BeforeServerRestart.php`:
- Around line 9-11: Add the required title-only Laravel-style docblock
immediately above the __construct method in BeforeServerRestart, describing the
constructor without parameter or return annotations. Leave the constructor
signature and behavior unchanged.

In `@src/watcher/src/ServerRestartStrategy.php`:
- Around line 109-110: Update the BeforeServerRestart dispatch in the server
restart strategy to first check whether the events dispatcher has listeners for
that event via hasListeners(). Only construct and dispatch BeforeServerRestart
when listeners are present; otherwise skip the event entirely.
- Around line 91-95: Update the PID-file handling in ServerRestartStrategy so
the check and read cannot fail when the server removes the file between
operations. Replace the separate filesystem exists/get sequence with the
available atomic or exception-safe read approach, while preserving the existing
empty/missing PID behavior so Watcher::run cleanup can continue to restart.

In `@tests/Cache/CacheSwooleStoreConcurrencyTest.php`:
- Around line 241-275: Update the child process closure in the concurrency test
to wrap all setup, callback, payload handling, and writeChildPayload operations
in a try/finally block, placing the existing SIGKILL call in finally. Ensure
exceptions from createStore, beforeReady, or writeChildPayload still
force-terminate the forked child while preserving the current payload behavior
for callback exceptions.

In `@tests/Coordinator/TimerTest.php`:
- Around line 132-146: Widen only the first callback wait in
testTickClearedFromItsCallbackDoesNotWaitAnotherInterval by increasing the
called->pop timeout beyond 0.2 seconds to tolerate CI scheduling delays. Keep
the subsequent short usleep and Timer::stats assertion unchanged so it still
detects an extra scheduled interval.

In `@tests/Foundation/Exceptions/Renderer/ListenerContextIsolationTest.php`:
- Around line 36-45: Add the required : void return type to the modified test
method containing the parallel query assertions in ListenerContextIsolationTest,
while leaving its existing test logic unchanged.

In `@tests/Foundation/Testing/DatabaseConnectionResolverTest.php`:
- Around line 44-53: Wrap the container replacement, cached-connection reset,
assertion, and restoration in a try/finally block within the test, and restore
the original container via Container::setInstance($this->app) in finally so it
runs even when resetCachedConnections or the assertion fails.

In `@tests/Integration/Horizon/Feature/WorkerProcessTest.php`:
- Around line 43-45: Update the callback passed to WorkerProcessTest’s
$process->start call so the stream-type parameter $type is either used when
handling output or renamed according to the repository’s accepted
ignored-parameter convention, eliminating the unused-parameter warning while
preserving output capture.

In `@tests/ObjectPool/ChannelTest.php`:
- Around line 143-153: Synchronize the coroutine created in the channel wait
test before calling push(), ensuring the waiter has registered with wait(1.0)
first. Update the setup around SwooleCoroutine::create and waitResult so the
subsequent push/pop sequence reliably exercises the helper-coroutine fallback
path.

In `@tests/Queue/QueueWorkerTest.php`:
- Around line 139-147: Add the required : void return type to the
testWorkerCanMonitorTimeoutJobs() method declaration. Preserve the existing test
setup and assertions, and do not alter the worker behavior being tested.

---

Nitpick comments:
In `@src/foundation/src/Testing/DatabaseConnectionResolver.php`:
- Line 58: Update the docblocks for resetCachedConnections() and
flushCachedConnections() to include a “Tests only.” warning describing the
cross-coroutine stale/discarded-connection failure mode. Keep the flush() and
connection() method docblocks title-only, without adding explanatory text or
warnings.
- Around line 39-44: Add a public static flushState() method to
DatabaseConnectionResolver that delegates cleanup to the terminal wrapper
mechanism, ensuring the pooledConnections cache is cleared. Update
AfterEachTestSubscriber to invoke DatabaseConnectionResolver::flushState()
during per-test cleanup, following the framework static-state lifecycle.

In `@src/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.php`:
- Around line 250-256: Update the docblock for the reconnect-state method near
the Redis scaling provider to retain only its title and the required
`@phpstan-impure` annotation; remove the explanatory paragraph about Redis I/O,
Sleep, and disconnect().

In `@src/routing/src/CompiledRouteCollection.php`:
- Around line 235-288: Update the docblocks for getByName and getRoutes in
CompiledRouteCollection to use title-only Laravel-style descriptions. Remove the
cache-behavior explanatory paragraphs while retaining required static-analysis
annotations such as the getRoutes return type.

In `@src/testing/src/PHPUnit/AfterEachTestSubscriber.php`:
- Around line 48-50: Update the try block in AfterEachTestSubscriber to import
Hypervel\Foundation\Testing\DatabaseConnectionResolver with a use statement and
call flushCachedConnections() through the short DatabaseConnectionResolver name
instead of the fully qualified class name.

🪄 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: e2de83a5-9516-4fa1-a023-cb710c5fd232

📥 Commits

Reviewing files that changed from the base of the PR and between 0ab6e9d and ba8cfee.

📒 Files selected for processing (132)

AGENTS.md
docs/ai/differences-vs-laravel.md
docs/plans/2026-07-01-fortify-passkeys-port.md
docs/plans/2026-07-03-fortify-otphp-chillerlan-refactor.md
docs/plans/2026-07-04-testing-after-each-cleanup-registrars.md
docs/plans/2026-07-12-test-suite-lifecycle-and-concurrency-robustness.md
src/boost/docs/coroutines.md
src/boost/docs/testing.md
src/cache/src/SwooleTableState.php
src/concurrency/src/CoroutineDriver.php
src/console/src/Application.php
src/console/src/SignalRegistry.php
src/contracts/src/Pool/ConnectionInterface.php
src/contracts/src/Pool/PoolInterface.php
src/coordinator/src/Timer.php
src/core/src/Bootstrap/WorkerExitCallback.php
src/coroutine/src/Concurrent.php
src/coroutine/src/Coroutine.php
src/coroutine/src/Parallel.php
src/coroutine/src/WaitConcurrent.php
src/coroutine/src/functions.php
src/database/src/Pool/PooledConnection.php
src/engine/src/Coroutine.php
src/engine/src/Exceptions/CoroutineCreateException.php
src/engine/src/Http/Server.php
src/engine/src/SafeSocket.php
src/foundation/src/Console/ConfigCacheCommand.php
src/foundation/src/Console/RouteCacheCommand.php
src/foundation/src/Testing/Concerns/InteractsWithTestCaseLifecycle.php
src/foundation/src/Testing/Concerns/RunTestsInCoroutine.php
src/foundation/src/Testing/DatabaseConnectionResolver.php
src/foundation/src/Testing/TestCase.php
src/horizon/src/Console/HorizonRestartStrategy.php
src/horizon/src/ListensForSignals.php
src/horizon/src/MasterSupervisor.php
src/horizon/src/ProcessPool.php
src/horizon/src/Supervisor.php
src/horizon/src/SupervisorProcess.php
src/horizon/src/WorkerProcess.php
src/object-pool/src/Channel.php
src/object-pool/src/ObjectPool.php
src/pool/src/Channel.php
src/pool/src/Connection.php
src/pool/src/Frequency.php
src/pool/src/KeepaliveConnection.php
src/pool/src/Pool.php
src/prompts/src/Spinner.php
src/prompts/src/Task.php
src/queue/src/Worker.php
src/redis/src/RedisConnection.php
src/redis/src/Subscriber/CommandInvoker.php
src/reverb/src/Servers/Hypervel/Contracts/PubSubProvider.php
src/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.php
src/reverb/src/Servers/Hypervel/Scaling/SwooleTableSharedState.php
src/routing/src/CompiledRouteCollection.php
src/routing/src/Router.php
src/signal/src/SignalManager.php
src/testbench/src/Bootstrapper.php
src/testbench/src/PHPUnit/TestCase.php
src/testing/src/Concerns/InteractsWithMockery.php
src/testing/src/PHPUnit/AfterEachTestSubscriber.php
src/watcher/README.md
src/watcher/src/Driver/AbstractDriver.php
src/watcher/src/Driver/DriverInterface.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/RestartStrategy.php
src/watcher/src/ServerRestartStrategy.php
src/watcher/src/WatchPath.php
src/watcher/src/Watcher.php
tests/Cache/CacheSwooleStoreConcurrencyTest.php
tests/Console/ConsoleApplicationProgrammaticTest.php
tests/Console/SignalRegistryCreateFailureTest.php
tests/Coordinator/TimerTest.php
tests/Core/Bootstrap/WorkerExitCallbackTest.php
tests/Coroutine/CoroutineCreateFailureTest.php
tests/Coroutine/CoroutineTest.php
tests/Coroutine/FunctionTest.php
tests/Engine/CoroutineCreateFailureTest.php
tests/Engine/HttpServerTest.php
tests/Engine/SocketTest.php
tests/Foundation/Exceptions/Renderer/ListenerContextIsolationTest.php
tests/Foundation/Testing/DatabaseConnectionResolverTest.php
tests/Foundation/Testing/UnitTestTest.php
tests/Horizon/Console/InstallCommandTest.php
tests/Integration/Foundation/Console/ApiInstallCommandTest.php
tests/Integration/Foundation/Console/BroadcastingInstallCommandTest.php
tests/Integration/Foundation/Console/ConfigCacheCommandTest.php
tests/Integration/Foundation/Console/RouteCacheCommandTest.php
tests/Integration/Generators/ProviderMakeCommandTest.php
tests/Integration/Horizon/Feature/Fixtures/EternalSupervisor.php
tests/Integration/Horizon/Feature/ListenCommandTest.php
tests/Integration/Horizon/Feature/MasterSupervisorTest.php
tests/Integration/Horizon/Feature/SupervisorTest.php
tests/Integration/Horizon/Feature/WorkerProcessTest.php
tests/Integration/Redis/RedisSubscribeIntegrationTest.php
tests/Integration/Redis/Subscriber/SubscriberIntegrationTest.php
tests/ObjectPool/ChannelTest.php
tests/ObjectPool/ObjectPoolNonCoroutineTest.php
tests/ObjectPool/ObjectPoolTest.php
tests/Pool/ChannelTest.php
tests/Pool/ConnectionTest.php
tests/Pool/FrequencyTest.php
tests/Pool/HeartbeatConnectionTest.php
tests/Pool/PoolNonCoroutineTest.php
tests/Pool/PoolTest.php
tests/Prompts/CoroutineCreateFailureTest.php
tests/Queue/QueueWorkerTest.php
tests/Redis/Subscriber/CommandInvokerCreateFailureTest.php
tests/Reverb/Servers/Hypervel/Scaling/RedisPubSubProviderTest.php
tests/Reverb/Servers/Hypervel/Scaling/SwooleTableSharedStateLockTest.php
tests/Routing/RoutePortTest.php
tests/Signal/SignalManagerCreateFailureTest.php
tests/Telescope/Console/InstallCommandTest.php
tests/TestCase.php
tests/Testbench/BootstrapperTest.php
tests/Testbench/Foundation/Process/RemoteCommandTest.php
tests/Testing/CleanupActionsTest.php
tests/Testing/Concerns/InteractsWithMockeryTest.php
tests/Testing/Fixtures/CleanupActions.php
tests/Testing/PHPUnit/AfterEachTestSubscriberTest.php
tests/Watcher/Driver/FindDriverTest.php
tests/Watcher/Driver/FindNewerDriverTest.php
tests/Watcher/Driver/FswatchDriverTest.php
tests/Watcher/Driver/ScanFileDriverTest.php
tests/Watcher/Fixtures/FindNewerDriverStub.php
tests/Watcher/Fixtures/FswatchDriverStub.php
tests/Watcher/ServerRestartStrategyTest.php
tests/Watcher/WatcherTest.php

💤 Files with no reviewable changes (2)

docs/ai/differences-vs-laravel.md
src/reverb/src/Servers/Hypervel/Contracts/PubSubProvider.php

greptile-apps · 2026-07-12T09:09:36Z

Greptile Summary

This PR systematically hardens lifecycle and concurrency contracts across Hypervel's coroutine, pool, watcher, queue, signal, and testing layers. The core change is a uniform coroutine-creation contract: native failures now throw CoroutineCreateException instead of returning false or −1, and every stateful caller rolls back its pre-spawn reservations transactionally.

Coroutine contract and callers: Engine\Coroutine::execute() converts Swoole's false to a typed exception; Concurrent, Parallel, Timer, SignalManager, Prompts\Spinner/Task, and Reverb pub/sub all roll back their pre-spawn state on failure. The HTTP server returns HTTP 503 on exhaustion.
Pool discard and checkout: Pool::discard() destroys a borrowed connection while restoring its capacity slot; getConnection() performs one extra state pass at its deadline to close the timeout-vs-release race; the testing DatabaseConnectionResolver retains each borrowed wrapper until teardown and discards it properly.
Watcher and queue lifecycle: Polling drivers use a stop-channel instead of detached timers; Watcher::run() observes driver completion and drains final batches; the queue worker owns one injected Timer for the monitor, clears it in finally, and kill() is now a hard-terminate path that dispatches WorkerStopping then sends SIGKILL immediately.

Confidence Score: 5/5

Safe to merge — the changes are disciplined lifecycle hardening with no regressions identified across coroutine, pool, watcher, queue, or test teardown paths.

The coroutine creation contract change is consistent across all callers (Concurrent, Parallel, Timer, SignalManager, Prompts, Reverb pub/sub, Channel.signal). Pool discard correctly removes from both managed/borrowed maps and signals waiters through the existing destroyConnection() path. The watcher stop-channel model eliminates the detached-timer lifetime problem. The queue worker's owned Timer clears in finally. The DatabaseConnectionResolver now retains and explicitly discards its pooled wrappers. The testing teardown uses capture() to run every independent cleanup action even after earlier failures. The PR's stated behavioral changes (hard kill for timeout workers, per-instance route cache, console programmatic path bypass) are deliberate and correctly scoped.

No files require special attention — the most complex changes (RedisPubSubProvider ownership sequencing, Pool.getConnection() deadline pass, DatabaseConnectionResolver dual-flush) all check out on close reading.

Important Files Changed

Filename	Overview
src/engine/src/Coroutine.php	Converts Swoole's false return from SwooleCo::create() into CoroutineCreateException via @-suppressed error capture and fromLastError(); getId() can now be called unconditionally.
src/engine/src/Exceptions/CoroutineCreateException.php	New exception class with fromLastError() factory using swoole_last_error()/swoole_strerror(); correctly extends RuntimeException.
src/engine/src/Http/Server.php	Extracts request dispatch to dispatchRequest(); catches CoroutineCreateException and returns HTTP 503, preventing coroutine exhaustion from crashing the server callback.
src/engine/src/SafeSocket.php	Fixes recvAll/recvPacket to check === false
src/coroutine/src/Concurrent.php	create() and fork() wrap Coroutine::create/fork in try-catch, popping the capacity channel on failure before re-throwing — correctly rolls back the pre-spawn push.
src/coroutine/src/Parallel.php	wait() adds a catch block for failed spawns: records throwable, pops concurrentChannel, calls wg->done() — maintaining the pre-added total WG count invariant.
src/pool/src/Pool.php	Adds discard() that calls destroyConnection() (closes connection, frees capacity, signals waiters); getConnection() defers throw until one extra state pass after deadline; assertBorrowed() includes the operation name in error messages.
src/database/src/Pool/PooledConnection.php	Adds discard() delegating to pool; ping() now uses CoroutineCreateException instead of false check; close() always calls disconnect() (pool retains shared in-memory SQLite PDO reference independently).
src/foundation/src/Testing/DatabaseConnectionResolver.php	Retains borrowed PooledConnection wrappers in $pooledConnections alongside bare connections; splits flush into reset (per-test) and discard (teardown); flush(name) discards the wrapper before clearing; discardCachedConnections() preserves first exception.
src/watcher/src/Watcher.php	run() observes driver completion via WaitGroup(1); drains final batch after driver exits; calls driver.stop(), strategy.stop(), channel.close(), and 1-second join in finally with capture() isolation.
src/watcher/src/Driver/FswatchDriver.php	Removes per-batch coroutine spawning; accumulates partial reads in a buffer; processOutput() processes complete lines inline and handles final flush; shouldStopWatching() now checks $this->stopping first.
src/queue/src/Worker.php	Replaces monitorTimeoutJobs callable seam with injected Timer; wraps daemon loop in try-finally to always clear the monitor timer; kill() now sends SIGKILL immediately (hard-terminate path); listenForSignals() unblocks HANDLED_SIGNALS via pcntl_sigprocmask.
src/coordinator/src/Timer.php	after() and tick() resolve the coordinator before spawning, wrap go() in try-catch, unset closure and re-throw on failure; while loop now checks isset(closures[id]) at the top.
src/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.php	connect() handshakes subscribe before spawning consumer; ownership tracked through handshake/drain/spawn/disconnect/reconnect; clearSubscriber/closeSubscriber guard against stale ownership; processQueuedPublishes() drains one-at-a-time with shouldRetry() checks.
src/routing/src/CompiledRouteCollection.php	$cachedRoutesByName changed from static (worker-lifetime, cross-collection) to instance (per-collection); flushCache() removed; AfterEachTestSubscriber updated accordingly.
src/console/src/Application.php	call() now routes through runProgrammatically() + configureProgrammaticIO() instead of Symfony's run(), preventing inheritance of process-global shell verbosity while preserving explicit IO option semantics.
src/cache/src/SwooleTableState.php	acquire() adds bounded 1-second timeout with hrtime(); withAllRowLocks() moves the acquire loop inside try so partial acquisitions are released on timeout.
src/signal/src/SignalManager.php	listen() tracks spawned coroutine IDs; on spawn failure, cancels all already-started signal watchers before re-throwing — prevents dangling watchers when partial spawn fails.
src/testing/src/PHPUnit/AfterEachTestSubscriber.php	flushStateAfterTest() now isolates each cleanup step (AfterEachTestCleanup, Mockery::close, flushCachedConnections, flushFrameworkState) with individual try-catch; Mockery::close() moved out of flushFrameworkState.
src/horizon/src/WorkerProcess.php	start() blocks STARTUP_SIGNALS before fork/exec and restores parent mask in finally; kill() added for hard process termination; event dispatches guarded with hasListeners().

_{Reviews (2): Last reviewed commit: "Tighten lifecycle regression tests" | Re-trigger Greptile}

Validate native file reads before returning from the typed filesystem API and convert unreadable or vanished files into the existing framework exception contract.\n\nPreserve the file session driver's empty-session semantics when a session is concurrently removed between its metadata check and locked read. Add regressions for both locked and unlocked filesystem races and the session-handler adaptation.

Treat skeleton copying, environment setup, and process-marker creation as one initialization transaction.\n\nReject partial directory copies, roll back unpublished runtime directories on every creation failure without masking the primary exception, and verify both copy and marker failures through the existing filesystem seam.

Move child self-termination into finally blocks for the cache concurrency harness and Reverb lock regression.\n\nThis prevents exceptional child setup or payload writes from reaching PHPUnit and Testbench shutdown handlers inherited from the parent process and deleting shared test infrastructure.

Keep the earliest teardown failure primary when aggregate framework state cleanup also fails, while retaining terminal database wrapper cleanup as an independently captured resource operation.\n\nClarify the resolver's process-global test-only lifecycle, use the imported resolver at the subscriber boundary, and restore a swapped global container reliably from test failures.

Eliminate the watcher pid-file check/read race and tolerate a pidfile disappearing while the server exits.\n\nGuard all optional watcher and Horizon restart events through the typed dispatcher so event construction and dispatch are skipped without listeners, while preserving EventFake and registered-listener behavior. Add focused listener and disappearance regressions.

Give the timer callback regression enough scheduling margin under parallel CI without weakening its no-reschedule assertion.\n\nComplete the required void return types on the modified coroutine-isolation and queue-worker monitor tests.

binaryfire · 2026-07-12T10:21:46Z

@greptileai @coderabbitai Thanks for the detailed review. I verified every inline, summary, nitpick, and outside-diff item against the current lifecycle and pushed the accepted fixes.

Addressed

Testbench runtime creation is now transactional. A failed directory copy, environment setup, or process-marker write removes the unpublished partial runtime without masking the creation failure. The previously ignored copyDirectory() result is now checked.
The cache concurrency child always self-terminates from finally, including setup and payload-write failures. The same inherited-shutdown-handler defect was found and fixed in the Reverb lock child.
The watcher now performs one exception-safe pidfile read, so a server removing its pidfile during shutdown is treated as already stopped.
That review exposed the lower-level form of the same race: Filesystem::get() and sharedGet() could return native false through declared : string methods, producing a warning plus TypeError, while locked open failure was conflated with an empty file. Both now honor the existing string-or-FileNotFoundException contract. FileSessionHandler explicitly preserves fresh-session behavior when concurrent GC or destroy removes the file between metadata validation and the locked read.
Optional BeforeServerRestart dispatch is guarded by hasListeners(). The two sibling Horizon lifecycle events now follow the same rule through a statically typed dispatcher.
Aggregate framework-state cleanup now preserves an earlier callback, Mockery, or database teardown failure. Terminal database wrapper cleanup remains separate from the no-throw static reset list.
The database resolver uses the imported type, documents its test-only process-global mutators, and its container-swap regression restores the actual prior container from finally.
The timer regression has additional CI scheduling margin without weakening the no-reschedule assertion.
The two modified test methods now carry their required : void types, and BeforeServerRestart has the conventional constructor docblock.
The queue worker's configured monitor interval is now carried into WorkerOptions and covered by the worker regression.

Intentionally unchanged

The object-pool waiter regression needs no additional synchronization. Swoole runs the created coroutine until its first yield, and Channel::wait() increments the waiter count before yielding. The fallback path is deterministic.
Fswatch empty non-EOF reads remain terminal. Its pipe is blocking, the condition is an explicit read failure in the tests, and retrying without a readiness transition risks a CPU spin.
The one-second watcher join remains a fixed lifecycle assertion. Driver shutdown actively kills/closes fswatch resources or closes polling stop channels before joining; it is not waiting for future fswatch output, and cleanup cannot replace an existing primary failure.
The Horizon callback's $type argument remains descriptively named because it is the required positional Symfony callback parameter. PHPMD is not a project gate and there is no project underscore convention to apply.
The watcher interface, Reverb impurity, and compiled-route cache docblocks remain. They document non-obvious lifecycle, coroutine-yield, and route-identity contracts; AGENTS.md permits explanatory bodies for complex methods.
No DatabaseConnectionResolver::flushState() alias was added. Its terminal wrapper cleanup performs resource ownership work and is intentionally captured outside the authoritative no-throw static reset list.
No SIGCHLD comment was added. SIGCHLD is absent from STARTUP_SIGNALS, so the temporary block does not block it; restoring the prior mask correctly preserves any pre-existing parent choice. The suggested "delivered after unblocking" explanation would be inaccurate.
The incidental uniqid() warning is not security-relevant; the value is only a local timer identifier.

The complete follow-up passes formatting, static analysis, the full parallel suite, Testbench contract tests, and package dogfood tests.

greptile-apps · 2026-07-12T10:22:17Z

Thorough and well-reasoned follow-up — thank you for working through each item individually.

The addressed fixes are exactly right. The transactional runtime creation, finally-guarded child self-termination in both cache and Reverb lock children, the Filesystem string-or-exception contract correction, and the hasListeners() guard on BeforeServerRestart are all meaningful correctness improvements that go beyond the surface observations.

The intentionally unchanged items hold up on re-examination:

The object-pool waiter path is deterministic given Swoole's run-to-first-yield guarantee — no additional synchronization needed.
Terminating on an empty non-EOF read from a blocking fswatch pipe is the right call; retrying without a readiness transition is exactly how you get a CPU spin.
The one-second watcher join is an assertion that active shutdown completes, not a speculative wait — the distinction matters.
The SIGCHLD absence from STARTUP_SIGNALS makes the mask behavior self-documenting; adding an explanation of the delivered-after-unblocking model would have been inaccurate for this specific case.

This is in good shape. The lifecycle contracts are now consistently enforced at ownership boundaries, and the test suite exercises them directly rather than through timing. Happy to approve.

coderabbitai · 2026-07-12T10:22:20Z

Tip

For best results, initiate chat on the files or code changes.

@binaryfire Thanks for the thorough write-up and for working through the entire list so systematically — the transactional Testbench runtime creation, the finally-based self-termination fix for both the cache concurrency and Reverb lock children, the Filesystem::get()/sharedGet() false-vs-string contract fix, and the aggregate framework-state cleanup preserving the earliest failure are all solid hardening improvements that go beyond the original review scope.

The rationale for the intentionally-unchanged items is convincing:

Swoole's run-to-first-yield semantics genuinely make the object-pool waiter registration ordering deterministic without extra synchronization.
Treating fswatch empty non-EOF reads as terminal is the right call given the blocking pipe and spin risk.
The fixed one-second watcher join is reasonable since shutdown is driven by active resource closure, not by waiting on driver output.
SIGCHLD being absent from STARTUP_SIGNALS is a good catch — that does correctly avoid the inaccurate "delivered after unblocking" framing.

Nice work getting the full parallel suite, Testbench contract tests, and dogfood tests green through all this.

ᕦ(ツ)ᕤ

✏️ Learnings added

Learnt from: binaryfire
URL: https://github.com/hypervel/components/pull/430

Timestamp: 2026-07-12T10:22:19.693Z
Learning: In the Hypervel components repo, PHPMD is not a project-enforced static analysis gate, and there is no project convention requiring underscore-prefixed unused parameter names (e.g., Horizon signal callback `$type` parameters intentionally keep descriptive names even if unused, since it's a required positional Symfony callback parameter).

Learnt from: binaryfire
URL: https://github.com/hypervel/components/pull/430

Timestamp: 2026-07-12T10:22:19.693Z
Learning: In the Hypervel components repo, `src/watcher/src/Driver/AbstractDriver.php`'s `stop()` method and related watcher/driver join logic use a fixed ~1 second shutdown timeout because driver shutdown actively kills/closes underlying resources (e.g., fswatch process, polling stop channel) before joining rather than waiting on future driver output; this is an intentional fixed lifecycle assertion, not a race-prone timing hack.

Learnt from: binaryfire
URL: https://github.com/hypervel/components/pull/430

Timestamp: 2026-07-12T10:22:19.693Z
Learning: In the Hypervel components repo, `Concurrent`/object-pool waiter registration ordering does not require additional synchronization beyond Swoole's coroutine scheduling guarantees: Swoole runs a newly created coroutine until its first yield point, and `Channel::wait()` increments the waiter count before yielding, making the fallback/registration path deterministic.

Learnt from: binaryfire
URL: https://github.com/hypervel/components/pull/430

Timestamp: 2026-07-12T10:22:19.693Z
Learning: In the Hypervel components repo (`src/horizon/src/WorkerProcess.php` and similar), `STARTUP_SIGNALS` constants intentionally exclude `SIGCHLD`; temporarily blocking `STARTUP_SIGNALS` during startup does not block `SIGCHLD`, and restoring the prior signal mask afterward correctly preserves any pre-existing parent signal-blocking choice.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

binaryfire added 30 commits July 12, 2026 08:10

coderabbitai Bot reviewed Jul 12, 2026

View reviewed changes

greptile-apps Bot reviewed Jul 12, 2026

View reviewed changes

Comment thread src/watcher/src/Driver/FswatchDriver.php

Comment thread src/watcher/src/Watcher.php

binaryfire added 6 commits July 12, 2026 10:18

Tighten lifecycle regression tests

88b864e

Give the timer callback regression enough scheduling margin under parallel CI without weakening its no-reschedule assertion.\n\nComplete the required void return types on the modified coroutine-isolation and queue-worker monitor tests.

binaryfire merged commit e9c038c into 0.4 Jul 12, 2026
36 checks passed

binaryfire deleted the fix/test-suite-lifecycle-robustness branch July 21, 2026 09:08

This was referenced Jul 23, 2026

fix: harden WebSocket transport and watcher process lifecycles #451

Merged

Harden Redis pooling, subscriber transport, and topology #453

Merged

Uh oh!

Uh oh!

Conversation

binaryfire commented Jul 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Coroutine creation

Cleanup and test ownership

Pools and testing database connections

Queue and Horizon process lifecycle

Watcher lifecycle

Additional correctness fixes

Compatibility

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

binaryfire commented Jul 12, 2026

Addressed

Intentionally unchanged

Uh oh!

greptile-apps Bot commented Jul 12, 2026

Uh oh!

coderabbitai Bot commented Jul 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

binaryfire commented Jul 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 12, 2026 •

edited

Loading

greptile-apps Bot commented Jul 12, 2026 •

edited

Loading