Clarify pool APIs and fix resource lifecycles - #39
Conversation
Move the duplicate object and connection pool channels into Coroutine\PoolChannel. Keep the resource queue separate from native wake signals so queued resources remain available for draining after close. Consolidate channel tests while retaining FIFO behavior, cancellation classification, waiter cleanup, signal coalescing and coroutine/non-coroutine transitions.
Expose the two pool families through Hypervel\Contracts\ObjectPool and Hypervel\Contracts\ConnectionPool without Interface suffixes. Define consistent borrowing, options, counts, snapshots and lifecycle operations while preserving the distinct object factory contract. Replace the split frequency interfaces with UsageTracker and remove the mutable options contract. Keep optional concrete signature types without introducing reverse package dependencies.
Rename the package, namespace, base class, release event and tests to make protocol connection pooling explicit. Update root autoloading and package replacement metadata, and introduce immutable PoolOptions with nullable disabled durations and precise closed/exhausted failures. Preserve capacity reservations and strict ownership while fixing cancellation during acquisition, release and idle disposal. Add explicit background activation, an owned optional idle monitor and a monotonic borrow-rate tracker with bounded sampling and a running total. Make keepalive reconnect, heartbeat and close cleanup respect the socket channel they own, including concurrent replacement and canceled waits. Retain protocol extension points and cover lifecycle, timing, cancellation and non-coroutine behavior in the renamed test suite.
Rename whole-pool idle_ttl to pool_idle_timeout and use null to disable optional idle, lifetime and pool inactivity limits. Keep finite positive validation, strict count validation and immutable option equivalence. Update option tests to cover defaults, explicit null, invalid sentinels and equivalent normalized definitions.
Rename SimpleObjectPool to CallbackObjectPool and expose borrow(), getOptions(), managed counts and pool inactivity consistently. Adopt the shared PoolChannel and central contracts without merging object lifetime policy into protocol connection behavior. Distinguish pool exhaustion from terminal closure, retain strict release/discard ownership and preserve deferred lease cleanup. Migrate callback, lease, lifecycle and non-coroutine tests with the source.
Use getPools(), getDefinition(), purge() and purgeAll() for registry inspection and teardown. Detach selected entries before yielding cleanup and retain expected-instance protection for replacements. Attempt every detached pool even when one close fails, preserving cancellation precedence after the finite drain. Extend manager coverage for failure isolation, cancellation and replacement ownership.
Configure each recycler with an owned Timer and validated constructor interval. Remove mutable timer accessors while retaining start/stop idempotence and the existing worker lifecycle wiring. Pass cancellation through both maintenance catch boundaries so the timer can stop without maintaining further pools. Keep ordinary per-pool error isolation and update provider and recycler tests for constructor customization and service identity.
Move HasPoolProxy into Concerns and use central contracts, explicit creation callbacks and the poolableDrivers host convention. Keep getPoolName() consistently returning the registry name and preserve definition matching and lease semantics. Migrate proxy and concern tests and remove the object-pool package dependency on Engine now that channel ownership belongs to Coroutine.
Rename DbPool and PoolFactory to DatabasePool and PoolManager throughout database consumers and tests. Use the connection-pool package, immutable options, nullable deadlines and the explicit borrowing and purge APIs. Start maintenance only after container resolution succeeds. Recheck registry ownership after construction and after yielding loser cleanup, preserving physical read/write names and the current open winner. Close rejected activation candidates with defined cancellation precedence. Fix heartbeat disposal to destroy an owned resource once and clear the shared SQLite PDO even when close propagates cancellation. Extend publication, activation, teardown and SQLite coverage while retaining connection pinning and session behavior.
Replace Redis PoolFactory with PoolManager and migrate connection, proxy, listener and provider consumers to the new contracts and borrowing APIs. Preserve native Redis timeout, retry and coroutine pinning behavior with immutable nullable pool options. Activate heartbeat maintenance after accepted container resolution and use a publication loop that rechecks the registry after yielding cleanup. Separate health evaluation from disposal so cancellation cannot cause double destruction or continue a canceled sweep. Update Redis lifecycle tests and the cache, Reverb and Telescope consumers that inspect or configure Redis pools. Document the new pool configuration and manager surface.
Ship the renamed retention, heartbeat and pool inactivity keys with null-preserving environment inheritance. Keep the established subsystem prefixes and normalize nullable values without converting explicit null to zero. Migrate database and Redis test helpers to PoolManager and purgeAll(). Preserve teardown ordering so coroutine-deferred borrowers return before pools drain, and update configuration, bootstrap and cleanup tests.
Use central object-pool contracts and the new borrowing and manager APIs in pooled filesystems and proxies. Rename the Hypervel-specific poolable driver methods and update Storage facade annotations. Preserve leased stream and file response lifetimes, including cleanup after deferred use. Update filesystem coverage and documented pool options.
Update broadcasting package dependencies and Redis pool integration for connection-pool. Rename the Hypervel-specific poolable driver surface and adopt central object-pool contracts. Keep broadcaster selection and Laravel broadcasting behavior intact, with matching facade annotations and manager/proxy tests.
Migrate MailManager to central object-pool contracts and the consistent poolable driver and purge APIs. Preserve transport reuse and the existing failover, round-robin and provider behavior. Update Mail facade annotations, manager/transport tests and the documented null-disabled pool options.
Use the new object-pool contracts, borrowing and manager operations in queue managers and proxies. Rename the Hypervel-specific poolable driver methods and update Queue facade annotations. Preserve worker/job resource ownership and deferred release across supported queue drivers. Migrate worker, retry, job and SQLite lifetime coverage together with queue configuration examples.
Rename the SDK transport pool to HttpTransportPool and migrate storage and transport consumers to the central object-pool API. Normalize disabled Sentry pool lifetimes with null while preserving supported configuration keys. Skip telemetry only for expected pool exhaustion or closure; unexpected factory failures retain their error path. Inspect only existing Redis pools for span attributes so observation cannot create a replacement or start maintenance. Expose truthful managed, borrowed, idle and waiting attributes and update transport, configuration and integration tests. Clarify acquisition timeout behavior in the documentation.
Migrate instrumentation to the renamed pool packages and manager APIs. Derive used capacity as managed minus idle so maintenance and yielding destruction remain visible without an additional maintained counter. Keep existing metric identities, sampling guards and registry-based observation with no pool creation or network work. Update occupancy and Redis integration coverage and document metric identity considerations.
Explain the distinction between reusable object pools and protocol connection pools, including borrowing, release/discard, immutable configuration, manager operations and monitoring. Describe retention, nullable durations, explicit activation, extension points and resource counts with concise examples. Keep subsystem examples clearly identified and retain historical Hyperf credit without implying the independently maintained package tracks its design.
Replace the old pool package skeleton reference with websocket-server in the repository guide and Hyperf porting guide. The redesigned connection-pool package is independently maintained and no longer represents the structure of a direct Hyperf port.
Record the final package and API mapping, immutable options, lifecycle invariants, cancellation fixes, observability behavior and coordinated framework/skeleton migration. Include focused regression coverage and integration requirements for the public components change. Keep the document limited to the technical design and verification requirements.
Bring in the Laravel parity updates to database expression handling, queue lock ownership and command behavior, HTTP retry policies, mail rendering, framework helpers and type contracts. Preserve the connection-pool package rename and the serializable-closure dependency floor together in Composer metadata. Retain both sets of mail and queue documentation changes. The merge has no textual conflicts; incoming pool consumers and shared database, queue and testing boundaries require no additional source changes. Verify Composer metadata and autoloading, formatting, normal parallel PHPStan source and type-fixture analysis, and 1,188 focused tests. The full four-worker suite passes with 35,373 tests and 133,477 assertions; Testbench passes with 543 tests and dogfood passes with 6 tests. These suites retain their service and platform skips. Mark the public technical plan complete after integration verification.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
Greptile SummaryThis PR renames and separates the connection-pool and object-pool APIs, introduces immutable options and shared channel behavior, and revises resource acquisition, maintenance, observability, cancellation, and teardown lifecycles across database, Redis, filesystem, queue, Sentry, and OpenTelemetry integrations.
Confidence Score: 4/5The PR does not yet appear safe to merge because database and Redis managers can still publish a candidate that was closed during custom container initialization. The previous blocking finding remains outstanding. The changes since the previous review only document that custom resolution must return an open pool; Files Needing Attention: src/database/src/Pool/PoolManager.php, src/redis/src/Pool/PoolManager.php
|
| Filename | Overview |
|---|---|
| src/database/src/Pool/PoolManager.php | Constructs and activates database pools outside the registry, but can still publish a candidate that a custom initialization hook closed without throwing. |
| src/redis/src/Pool/PoolManager.php | Implements concurrent Redis pool construction and loser cleanup, with the same unresolved closed-candidate publication gap. |
| src/connection-pool/src/ConnectionPool.php | Defines the renamed connection-pool ownership, maintenance, accounting, and teardown lifecycle. |
| src/object-pool/src/ObjectPool.php | Migrates object pooling to the revised borrow, release, discard, count, and lifecycle APIs. |
| src/connection-pool/src/KeepaliveConnection.php | Ties asynchronous socket operations and heartbeat cleanup to the channel instance that owns the socket. |
| docs/plans/2026-09-08-0816-pool-api-and-lifecycle-redesign.md | Clarifies that successful custom container resolution must provide an open pool. |
| src/docs/pools.md | Documents the explicit activation boundary and open-pool requirement for custom resolution. |
Reviews (2): Last reviewed commit: "Clarify the connection pool construction..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ($existing === null || $existing->isClosed()) { | ||
| return $this->pools[$poolName] = $pool; |
There was a problem hiding this comment.
A resolving or startup hook can close the newly constructed candidate without throwing. start() then returns, and this branch publishes and returns the candidate because it checks only the registry entry, not whether $pool is closed. The next database operation receives a closed pool and fails during borrow(), while the invalid candidate remains registered until another lookup replaces it. The Redis manager has the same publication gap at src/redis/src/Pool/PoolManager.php:108-109; verify that the candidate is still open before publishing it.
Knowledge Base Used: Asynchronous and realtime systems
There was a problem hiding this comment.
Confirmed the behavior in both managers when custom initialization closes the candidate or substitutes an already-closed pool. I’m keeping the manager code unchanged and clarifying the construction contract in ec5e162.
Successful container resolution must provide an open pool. If a custom factory keeps returning a terminal instance, a publication guard only moves the same PoolClosedException from borrow() to pool(); it does not restore a usable pool. Retrying construction can run forever with that factory.
The shipped construction and startup paths do not close the unpublished candidate. The reported cases require custom code to supply or expose it. I reproduced the closing-callback cases for database and Redis: borrowing fails before acquiring a resource, and closure leaves no live maintenance timer or managed connection.
The guide now states that custom resolution must provide an open pool or throw when initialization fails. It makes no additional cleanup promise: initialization owns any work it starts, while the manager handles activation failure after container resolution returns.
Document that successful container resolution must provide an open pool and that custom initialization failures must be reported by throwing. Keep the requirement on the resolved object because container extenders can substitute an instance while resolving callback return values are ignored. Align the technical plan with the guide while preserving the distinction between initializer-owned cleanup and manager-owned activation cleanup after resolution returns. No runtime guard or reconstruction retry is added: checking a terminal factory result earlier only moves the existing failure, and retrying a factory that always returns it cannot recover. Validate the documentation diff and the relevant container and pool lifecycle paths. Source, tests and dependencies are unchanged.
|
I checked the closed-candidate finding against both managers, the container callbacks, and the activation and cleanup paths. The behavior is reproducible when custom resolution supplies a terminal pool, but the proposed guard does not repair that factory. It moves the existing exception to an earlier call; automatic reconstruction could retry forever. The manager code remains unchanged. ec5e162 clarifies the construction contract in the pool guide and technical plan: successful container resolution must provide an open pool, and initialization failure must be reported by throwing. The inline reply explains the cleanup ownership and reproduction results. This update changes documentation only. The documentation diff checks pass, and the existing formatter, PHPStan, focused and full suites, Testbench, and dogfood results remain applicable. CodeRabbit skipped review because of the PR file and capacity limits; it did not produce code findings. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline --decorate --no-renames ec5e162e8
printf '\n--- changed files ---\n'
git diff-tree --no-commit-id --name-status -r ec5e162e8
printf '\n--- documentation diff ---\n'
git show --format= --no-ext-diff --unified=30 --no-renames ec5e162e8Length of output: 15488
The commit changes only the pool guide and the technical plan. It documents that custom container resolution must return an open pool. It also documents that initialization failures must throw and that managers must not retry construction failures. The proposed closed-candidate guard does not repair a custom factory that returns a terminal pool. It only changes where the existing failure occurs. Retrying reconstruction could loop indefinitely. I withdraw the finding. ✏️ Learnings added
You are interacting with an AI system. |
|
That distinction is valid. With the clarified construction contract, the manager is entitled to receive an open pool after resolution; a custom resolver that returns or closes a terminal pool is violating that contract. An |
1 participant
Pool updates
This PR gives Hypervel's two pool packages distinct names and consistent APIs, and fixes the resource-lifecycle problems found while making that change.
hypervel/object-poolmanages reusable objects.hypervel/connection-poolmanages protocol connections. The packages keep their different health, lifetime and reconnection behavior; they share the channel implementation used to wait for resources.These are deliberate changes to Hypervel's pool APIs for 0.4. There are no aliases for the old names. Supported Laravel APIs remain unchanged.
Package and class names. Rename
hypervel/poolandHypervel\Pooltohypervel/connection-poolandHypervel\ConnectionPool. Rename the basePooltoConnectionPool,PoolOptiontoPoolOptions,SimpleObjectPooltoCallbackObjectPool, andDbPooltoDatabasePool. Database and Redis usePoolManagerinstead ofPoolFactory. Sentry's SDK transport pool becomesHttpTransportPool, distinct from the asynchronousHttpPoolTransportthat consumes it.Contracts. Put the connection and object pool contracts under
Hypervel\Contracts\ConnectionPoolandHypervel\Contracts\ObjectPool, withoutInterfacesuffixes. Keep the object manager'sFactorycontract. Replace the split frequency contracts withUsageTracker; options are concrete immutable values rather than another interchangeable service.Borrowing and inspection. Use
borrow(),release()anddiscard()for resource ownership. KeepLease::get()for access to an object already held by a lease. Both pool families exposegetOptions(), managed/borrowed/idle/waiting counts, and a stats snapshot. Managed counts describe actual resources and exclude pending creation slots; capacity checks still include those slots. Pool names remain the public naming concept, and proxygetPoolName()consistently returns the registry name.Manager operations. Use
pool()to resolve a connection pool,getPools()to inspect the registry, andpurge()/purgeAll()to detach and close pools. Database'spurgeForConnection()handles the physical read/write variants of a configured connection. Keep existing-only object pool lookup, definition inspection and expected-instance checks when removing an entry.Immutable options. Configure pools through
PoolOptions::fromArray()and read public readonly properties. Renamemin_connectionstomin_retained_connections,heartbeattoheartbeat_interval, and objectidle_ttltopool_idle_timeout. Nullable durations usenullto disable the behavior; zero and negative sentinels are rejected. Connection, wait and heartbeat timeouts remain positive. Normalize options once, reject unknown keys, and preserve explicit null through environment-variable inheritance. Keep the established database and Redis environment prefixes.Maintenance. Rename connection
flush()totrimExcessIdle(): it closes idle connections above the retained minimum without checking their age. ObjecttrimIdle()andsweepExpired()keep their distinct idle-age and absolute-lifetime rules. The retained minimum does not trigger eager creation or guarantee a minimum after failures.Usage and periodic checks. Replace
FrequencywithBorrowRateTracker, using monotonic time, bounded timestamp buckets and a running count to remove repeated summation on checkout. Keep custom policies through a protected factory. ReplaceConstantFrequencywith an ownedIdleConnectionMonitor, enabled byidle_check_interval. It checks one idle connection per tick without refreshing application activity. Database and Redis retain their protocol-specific heartbeat sweeps. Both capabilities remain available without a shared maintenance framework.Activation and recycling. Start connection-pool maintenance explicitly after construction has been accepted. Directly constructed pools call
start()when background maintenance is wanted. Empty pools defer generic idle checks until their first successful borrow. Object recyclers keep constructor-configured intervals and optional timer injection, withstart()/stop()as their contract; mutable timer accessors are removed.Consumers and documentation. Migrate database, Redis, filesystem, broadcasting, mail, queue, Sentry and OpenTelemetry consumers, including test support and facade annotations. Use
poolableDriversconsistently for the Hypervel-specific driver-selection API. Document the new names, configuration, extension points, counts and lifecycle examples. The connection-pool package is independently maintained; historical Hyperf credit remains in the pool guide. The Hyperf porting reference now useswebsocket-server.The package rename also requires the companion
hypervel/frameworkdependency change and matchinghypervel/hypervelapplication-skeleton configuration. Those repositories must ship compatible changes together so generated applications do not supply removed option names or disabled-value sentinels.Additional Hypervel fixes
Concurrent pool construction. Database and Redis now construct candidates outside the registry and start maintenance after container resolution returns. A failed resolving callback cannot leave a rejected pool rooted by a timer. When two candidates compete, the manager closes the unused candidate and checks the registry again: cleanup can yield while the previous winner is closed or replaced. Activation failure cleans up the exact candidate without removing another registered pool.
Cancellation and disposal. Separate idle health evaluation from destruction so cancellation cannot cause a connection to be destroyed twice. A canceled sweep cleans up its current resource and stops. Preserve the original cancellation when secondary cleanup also fails. Apply the same boundary to generic idle checks and the shipped database and Redis heartbeat loops.
Acquisition and release. Remove the base connection's blanket retry of arbitrary failures. Drivers retain their own health and reconnect behavior. Dispatch
ConnectionReleasingbefore return and release the connection exactly once, including when a listener or its logger throws. Keep ownership errors distinct from expected pool closure or capacity exhaustion.Keepalive sockets. Tie pending calls, heartbeat cleanup and close operations to the socket channel they acquired. A late operation cannot requeue into, close, or clear a replacement connection. Canceled waiters preserve another caller's socket; explicit close clears its own state even when acquisition fails. Concurrent reconnects dispose of the unused socket and do not overwrite an active heartbeat timer. Replaced-channel waiters are woken after replacement state is ready.
Complete teardown. Detach manager entries before cleanup and attempt every selected pool even if one close fails. Preserve cancellation precedence after the finite drain. Clear the shared in-memory SQLite PDO reference in
finallywhen pool closure throws. Pass object recycler cancellation through to the timer instead of reporting it as an ordinary maintenance failure and continuing.Sentry failures and Redis spans. Add distinct closed/exhausted exceptions and drop telemetry only for those expected acquisition failures. Unexpected transport factory failures retain their error path. Redis tracing inspects existing pools instead of creating one during observation, so a command event after a purge cannot start a replacement pool and timer. Record truthful managed, borrowed, idle and waiting attributes while retaining the command span when no pool is registered.
OpenTelemetry occupancy. Calculate used resources as managed minus idle. Resources undergoing maintenance or yielding destruction still occupy capacity even when they are not borrowed by application code. Preserve the existing instruments and attributes without adding another maintained counter, health checks or network work to metric collection.
Verification includes Composer metadata and autoloading, the repository formatter, PHPStan source and type fixtures, focused pool and integration coverage, the full components suite, Testbench package-mode tests, and the external dogfood package. The combined code was checked after merging the current
0.4. Tests cover construction races, failed activation, cancellation, replacement ownership, complete cleanup, nullable configuration, observability and coroutine/non-coroutine use.