Rebuild object pool lifecycles and cloud filesystem pooling by binaryfire · Pull Request #429 · hypervel/components

binaryfire · 2026-07-11T08:03:33Z

Summary

Hypervel's existing pooling architecture did not model resource identity, object ownership, deferred lifetimes, invalidation, or coroutine/non-coroutine interaction rigorously enough for long-lived workers. Pools were primarily cached wrappers around factories, which allowed several classes of correctness problems: equivalent dynamic resources could collide, forgotten resources could leave stale pools behind, the same object could occupy multiple pool slots, and a proxy could return a stream, promise, iterator, or queue job after already releasing the resource that still backed it.

This PR rebuilds the pooling foundation around explicit invariants: immutable pool definitions, construction fingerprints, tracked ownership, exactly-once leases, deterministic closure, per-operation pool resolution, and bounded idle reclamation. The lower-level database and Redis connection pools adopt the same ownership and terminal lifecycle model.

Existing consumers are migrated to the corrected architecture. That migration also enables general-purpose dynamic resource use cases: S3 and GCS pool SDK clients independently of bucket-specific adapter stacks, on-demand mailers can opt into pooling, and scoped filesystem decorators can resolve a prefix per operation. These capabilities broaden Hypervel's support for dynamically constructed resources without coupling pools to application-level context or policy.

For more details, see: docs/plans/2026-07-10-object-pool-lifecycle-and-client-pooled-filesystems.md

Background

Pooling in a coroutine framework has a stricter contract than retaining a collection of reusable objects. A pool must know which objects it owns, which are currently borrowed, when a result still depends on a borrowed resource, and how callers recover when a pool is purged or evicted while work is in flight.

The previous implementation did not encode these properties directly:

Pool identity was derived from a manager and logical name. All on-demand filesystem builds therefore shared one identity, while a manager cache could forget a proxy without removing its pool.
Capacity was maintained as counters around a channel without object-identity tracking. Double releases, foreign releases, and factories returning an existing singleton could corrupt availability or expose the same object concurrently.
Coroutine and non-coroutine callers used separate object stores. An object released in one execution mode could be invisible to a borrower in the other while still consuming counted capacity.
Generic proxy forwarding released a borrowed object as soon as the method returned. This is unsound for streams, streamed responses, lazy listings, promises, and queue jobs that retain a backend client.
Recycling was unable to reclaim common single-object pools and did not remove abandoned pool registrations from the manager.
Cloud filesystem pools retained complete driver stacks even though the SDK client accounts for nearly all retained cost and bucket/prefix adapters are cheap wrappers.
Some consumers pooled stateless or container-shared objects rather than the resource that actually owns reusable state.

These were architectural issues rather than isolated call-site bugs. Fixing them locally would preserve the same failure modes for the next pooled consumer, so this PR makes the lifecycle rules part of the pool API itself.

Pool model

Definitions and identity

Managed pools are registered through an immutable PoolDefinition containing:

a namespaced identity;
a resource type;
a construction fingerprint; and
normalized PoolOptions.

Automatic fingerprints are produced from a canonical, type-tagged representation of the exact input used to construct the pooled resource. Map ordering does not affect identity, while different scalar types, enum types, list ordering, and resource types remain distinct. Configuration that cannot be canonicalized must declare an explicit fingerprint rather than silently converging.

PoolManager::getOrCreate() reuses a pool only when its resource type, fingerprint, and options match. Conflicting definitions fail immediately with a diagnostic explaining the mismatch. Closed registrations self-heal by detaching the closed instance and constructing a replacement.

Explicit pool names remain available, but they do not bypass construction-equivalence checks. Automatic and explicitly named identities occupy separate namespaces.

Ownership and capacity

Pools now track every managed object by identity and separately track borrowed objects and in-flight creation slots. This makes the following invariants enforceable:

only an object owned by the pool can be released or discarded;
a borrow can be finalized only once;
a factory must return a fresh object not already managed by the pool;
yielding factories cannot create beyond configured capacity;
a failed factory or discarded object wakes waiters that can use the newly available creation slot; and
borrowed objects released after closure are destroyed rather than returned to circulation.

Checkout uses one monotonic deadline across waiting, creation, and expired-object replacement. Duration arithmetic saturates safely for very large finite values instead of overflowing nanosecond calculations.

Leases and deferred results

Lease is the exactly-once finalization primitive for borrowed objects. A lease either releases a healthy object or discards a suspect object, and abandoned leases finalize defensively during destruction without allowing cleanup exceptions to escape.

Consumer proxies no longer expose generic public magic forwarding. They enumerate operations whose results are fully consumed during the borrow and use explicit lease-aware implementations for deferred work. Cleanup preserves the primary operation exception when finalization also fails.

Filesystem streams are wrapped by LeasedStream, which retains the client lease until the stream is explicitly closed or destroyed. Streamed file responses acquire their stream only when emission begins and close it in a finally block. Queue jobs retain their backend lease until their terminal delete, release, or bury operation completes.

Lifecycle and maintenance

Pools have an explicit, idempotent terminal close() operation. Closure rejects new borrows, wakes parked waiters, destroys idle objects, and causes late releases to be destroyed. Registries detach a pool before closing it so teardown that yields cannot expose a closing instance to another resolver.

The old strategy and ratio-based recycler has been replaced with direct lifecycle policies:

max_lifetime expires individual objects absolutely;
max_idle_time trims idle objects down to the retention floor; and
idle_ttl removes an entirely unused managed pool.

Maintenance requeues do not update user-activity timestamps. This prevents maintenance itself from keeping idle objects and pools alive forever. Maintenance and destructor-only reporting paths are no-throw and preserve bookkeeping even when resource cleanup fails.

The object and connection pool channels now use one canonical queue in all execution modes. A separate coalesced state signal wakes coroutine waiters without making storage dependent on the caller's coroutine state.

Connection pools

The lower-level Hypervel\Pool implementation now follows the same ownership, capacity, wait, and terminal-close semantics as the general object pool.

Database and Redis registries remove an exact pool instance before closing it. Heartbeat checks use the centralized destruction and requeue paths, do not reset idle clocks merely by probing a connection, and discard connections whose health check throws. Database closure also clears the shared in-memory SQLite PDO at the correct terminal boundary.

Connection pool options now reject invalid capacities, non-finite or non-positive durations, undocumented sentinel values, malformed event lists, and unknown keys before those values reach live timing or capacity arithmetic.

Cloud filesystems

Built-in S3 and GCS disks now pool SDK clients rather than complete filesystem adapters. Client construction input is selected explicitly and used both for fingerprinting and for constructing the client. Bucket, root, visibility, prefix, read-only, and response behavior remain per-disk adapter state and do not split a pool unnecessarily.

This means disks using the same credentials, region, endpoint, and client options can share connection resources while targeting different buckets or prefixes. Different credentials or client configuration produce different pools automatically. Repeated Storage::build() calls with equivalent configurations converge safely without a caller-provided identity key.

Every operation builds a cheap adapter stack around the borrowed client. Callback state is applied to that stack, so temporary URL and serving callbacks cannot leak between disks. Raw internals are available only through borrow-scoped withClient(), withAdapter(), and withDriver() callbacks.

The filesystem response path was also made range-aware and lease-safe. It now validates byte-range syntax, handles open-ended and suffix ranges correctly, caps emitted bytes, preserves a body containing "0", fails on read errors instead of spinning or truncating silently, and closes streams under both successful and failed output.

S3 ranged reads preserve sibling @http options, while GCS now follows the common throw/null behavior and shared Flysystem wrapping path. Direct filesystem paths are normalized before prefixing, and temporary URL callback registration supports static, first-class, and bound closure forms.

Dynamic scoped filesystems

ScopedFilesystemProxy and ScopedCloudFilesystemProxy add a strict dynamic-prefix boundary around an existing disk. The prefix resolver runs once per path operation, which allows request-scoped user, workspace, project, or sandbox prefixes without mutating manager configuration or process-global state.

The decorators fail closed when the resolved prefix is empty unless root passthrough was explicitly enabled. They normalize prefixes, reject traversal and control characters, validate the complete putFileAs() destination before performing I/O, strip returned paths defensively, and reject unknown forwarding that could bypass the scope.

Configuration and capability inspection methods that do not cross a path boundary pass through without requiring a request context.

Consumer migrations

Mail

Named poolable mailers retain their existing default pooling behavior. MailManager::build() is direct by default and can opt into pooling with pool: true, an empty array, or a pool options array.

Mail pool identity is derived from the complete transport construction input, including service credential fallbacks. Composite failover and round-robin transports fingerprint their recursively resolved children, so rotating a child credential produces a new pool. Nested composite transports remain direct within the pooled outer transport.

This supports dynamic provider credentials and subaccounts while retaining connection reuse, configured capacity limits, and idle eviction. The same work fixes round-robin retry_after selection and global addresses without a display name.

Queues

Pooled Beanstalkd and SQS jobs retain a lease on the queue backend after pop(). The lease is released only after a successful terminal backend operation; a failed backend operation discards the connection so a potentially desynchronized client is never reused.

Connection names are proxy state reapplied on every borrow, allowing equivalent logical connections to converge without leaking names. Job connection and queue names are total strings throughout the hierarchy, including framework fakes and synchronous jobs. Pool configuration metadata is no longer passed to connectors as resource construction input.

Broadcasting

Broadcaster pool identity is based on resource construction config rather than the logical connection name. Authenticated-user resolver callbacks are written on every borrow, including null, preventing callback state from leaking between proxies sharing a pool. Custom contract-only broadcasters remain usable unless an operation specifically requires a Hypervel base-broadcaster capability.

Reverb continues to resolve directly through its concurrency-safe shared SDK client. Existing Pusher and Ably pooling behavior is unchanged by this PR.

Notifications

Notification manager pooling has been removed. The Slack router is stateless and container-shared; the actual channel is resolved during send. Pooling that router managed no connection or mutable resource and could return the same auto-singleton object for multiple pool slots. Direct resolution preserves behavior and removes the ownership violation.

Custom notification channels that genuinely own reusable resources can use the general object-pool primitives within the channel implementation.

HTTP

HTTP client object pooling has been replaced with handler-level reuse. A Guzzle client is a stateless option container, while the low-level handler owns reusable cURL handles and keep-alive state.

Named connections now retain one low-level transport handler and immutable option presets. Every pending request creates a fresh middleware stack around that handler and owns its own cookie jar. This preserves transport reuse while preventing the first request's middleware or cookies from leaking into later requests.

Option layers have explicit precedence from factory globals through connection presets, per-call overrides, fluent request options, and explicit client or handler overrides. Reserved settings such as handler, cookies, and obsolete pool options are rejected at the boundary that owns them.

Sentry

Sentry's standalone transport pool adopts the rebuilt ownership contract and exposes only options it can enforce without managed maintenance. A transport that throws during send is discarded rather than returned to the next borrower as healthy.

API and configuration notes

General object pools use PoolOptions with min_retained_objects, max_objects, wait_timeout, max_lifetime, max_idle_time, and idle_ttl.
Managed resources may use pool.name for a readable explicit identity and pool.fingerprint to declare construction equivalence for configurations containing non-canonicalizable objects or callables.
Forget operations remain cache-only. Purge operations invalidate the underlying shared pool and retained proxies lazily reacquire a replacement.
Generic pooled proxy __call() behavior is intentionally removed where an unknown return value could outlive the borrow.
Filesystem macros and lazy raw listings are not exposed through pooled or scoped proxies; explicit safe methods and borrow-scoped accessors replace them.
HTTP connection configuration no longer creates object pools. Connection reuse is owned by the shared low-level handler.
Notification router pool configuration is removed because no resource was being pooled.

Hypervel 0.4 is unreleased, so this PR favors a coherent final architecture over compatibility shims for the previous internal pooling model.

Performance characteristics

The new ownership checks are constant-time object identity lookups on pool boundaries. Pool definitions and fingerprints are computed when proxies are constructed, not on every resource operation.

Cloud filesystem pooling now retains only the expensive SDK clients. Bucket and prefix adapter stacks are rebuilt per operation; they are small, stateless wrappers relative to the client and keep disk-specific state out of the shared pool. Equivalent dynamic disks therefore reuse connection resources without retaining a complete driver pool for every bucket or prefix.

HTTP retains connection reuse without borrowing clients from an object pool. Fresh middleware stacks and cookie jars isolate request state while the shared transport handler continues to own reusable connections and concurrent transfer machinery.

Maintenance is bounded by the number of idle objects currently present and no longer keeps resources alive by updating activity clocks during inspection.

Testing

Coverage includes pool identity and fingerprint canonicalization, ownership violations, concurrent yielding factories, cross-mode release and borrow behavior, waiter wakeups, terminal closure, late releases, idle trimming and eviction, cleanup failure reporting, and duration overflow boundaries.

Consumer tests cover cloud client convergence and separation, dynamic credentials and buckets, leased streams and streamed responses, range handling, scoped-path containment, on-demand mail pooling, composite transport fingerprints, queue job terminal ordering, failed-backend discard behavior, broadcaster callback isolation, direct notification routing, HTTP middleware and cookie isolation, handler reuse, and database and Redis teardown under coroutine execution.

The complete formatting, static analysis, parallel test, secondary package, and package dogfood gates pass.

Summary by CodeRabbit

New Features
- Identity-based object pooling with explicit lifecycle control (close/closed checks) and safer lease cleanup.
- Pooled cloud filesystem clients plus scoped filesystem proxies (fail-closed prefix security), streamed responses, and native HTTP byte-range support.
- Connection presets for HTTP with isolated per-request middleware and cookie jars; pooling coverage expanded across mail and queues.
Bug Fixes
- Fixed ranged/streaming edge cases and strengthened filesystem traversal protections; improved cleanup after failed or deferred operations.
Documentation
- Added object-pool and updated storage/HTTP/mail/queue/broadcast/notification guides; updated pool config keys (e.g., min_retained_objects, plus idle trimming controls).

Record the verified defects, design decisions, implementation details, test matrix, and finishing criteria for the object-pool lifecycle rebuild and dynamic resource pooling work. The plan captures the intended architecture as a durable reference for reviewers and future maintainers.

Document that Swoole channel operations can terminate fatally after the native runtime has been torn down. This makes the deterministic lifecycle requirement explicit and prevents future cleanup paths from invoking native channel methods from destructors or garbage collection.

Introduce normalized pool options, canonical construction fingerprints, and immutable resource definitions. Definitions make pool identity, resource type, construction equivalence, and lifecycle options explicit. Strict validation and deterministic hashing prevent typo-driven configuration drift and unsafe convergence across different resources.

Replace separate coroutine and non-coroutine object stores with one canonical queue and a state-change signal. Objects released in either execution mode are now visible everywhere, exhausted non-coroutine borrowers follow the normal failure path, and coalesced notifications wake waiters without risking an indefinitely blocking channel push.

Rebuild object checkout around explicit managed and borrowed identity tracking, reserved creation capacity, monotonic deadlines, deterministic closure, and no-throw destruction reporting. Add leases for exactly-once release or discard across synchronous and deferred work. Double releases, foreign objects, duplicate factory results, expired objects, suspended factories, and late releases after closure now have defined and regression-tested behavior.

Register pools through immutable definitions and verify resource type, construction fingerprint, and normalized options on reuse. Resolve pools per operation so retained proxies survive purge and idle eviction, remove unsafe public magic forwarding, preserve primary exceptions during finalization, and centralize namespaced automatic and explicit pool identities.

…tenance Replace ratio-based recycling with direct expired-object sweeping, idle trimming, and whole-pool idle TTL eviction. The recycler now removes exact registered instances safely, validates timer intervals, reports maintenance failures, and cannot reset user-activity clocks merely by inspecting idle objects. Obsolete strategy contracts and tests are removed.

Give connection pools the same canonical queue and non-blocking state signaling model as general object pools. Connections released inside or outside a coroutine remain mutually visible, waiters react to both released objects and freed capacity, and signaling is safely coalesced without indefinite pushes.

Reject invalid capacities, non-finite or non-positive durations, undocumented disable sentinels, malformed event lists, and unknown option keys before they reach live pool arithmetic. The validation covers constructors and mutable setters, preserving the established configuration surface while making configuration mistakes fail early with actionable messages.

Track managed, borrowed, and in-flight connections explicitly; reserve capacity before yielding factories; and replace ambiguous full flushing with deterministic terminal closure. Health-check failures now destroy suspect connections without stranding capacity, maintenance preserves idle clocks, late releases are destroyed after closure, and waiters wake on every capacity-relevant state transition.

Migrate database pools to deterministic closure and detach registry entries before teardown can yield, allowing concurrent resolvers to create a fresh pool immediately. Heartbeat maintenance now uses the central ownership paths, shared in-memory SQLite state is cleared only on terminal close, and parallel-safe scratch paths prevent workers from sharing database files.

Detach Redis pools before deterministic closure, remove generation bookkeeping made obsolete by terminal lifecycle semantics, and route heartbeat outcomes through the central ownership model. Healthy probes preserve idle timestamps, failed probes discard suspect connections, late releases cannot re-enter closed pools, and Horizon cleanup now invalidates pools through the factory lifecycle.

Run Redis-backed funnel cleanup inside the test coroutine so corrected cross-mode pool storage never hands a coroutine-created socket to outside-coroutine I/O. Clarify database teardown ordering around terminal pool closure and late releases while preserving the existing isolated cleanup coroutine.

Adapt Sentry's standalone transport pool to the rebuilt ownership API and expose only lifecycle options that an unmanaged pool can actually enforce. A transport that throws during send is now discarded rather than returned as healthy, teardown remains exactly once under coroutine defers, and unsupported maintenance-only configuration fails fast.

Normalize direct paths, make temporary URL callback handling support every valid closure kind, and provide a consistent ranged-stream contract across local, S3, and GCS adapters. Extract range-aware response construction, preserve exact byte limits and primary exceptions, fix malformed and suffix range handling, retain nested S3 HTTP options, align GCS failure semantics, and keep pooled clients leased until returned streams are closed.

Pool the expensive S3 and GCS SDK clients by their exact construction configuration while rebuilding cheap bucket, prefix, visibility, and adapter stacks for each operation. Add immutable pool convergence, safe on-demand builds, shared-resource purge semantics, borrow-scoped raw access, callback isolation, explicit pooled-disk method surfaces, scoped-parent expansion, and regression coverage for dynamic credentials and buckets.

Add per-operation scoped filesystem decorators for request, user, team, or tenant prefixes without mutating shared disk configuration. The boundary resolves each prefix once, fails closed for empty scopes, rejects traversal and unprefixed escape hatches, validates upload targets before I/O, strips returned paths defensively, and explicitly maps the complete safe filesystem surface.

Add explicit opt-in pooling for MailManager build operations while preserving named mailer defaults and direct construction for nested composite children. Pool identities derive from the complete resolved transport input, including service credential fallbacks and recursively resolved composite children. Purge, idle TTL reclamation, global-address handling, and round-robin retry configuration are corrected and covered.

Use non-null connection and queue names throughout job state, normalize nullable synchronous queue input at construction, and initialize framework fakes with meaningful defaults. This aligns concrete jobs with the queue job contract and prevents inherited accessors from reading uninitialized properties in application and Horizon test fixtures.

Attach an object-pool lease to jobs popped from pooled Beanstalkd and SQS connections so backend clients remain exclusively borrowed until delete, release, or bury completes. Successful terminal calls release the connection, failed protocol calls discard it, unsupported third-party jobs are requeued before failing closed, post-terminal client access is guarded, connection names are applied per borrow, and manager purge and application swaps invalidate pools safely.

Build broadcaster pools from resource construction input instead of logical connection names and reapply authenticated-user resolver state on every borrow, including explicit clearing. Purge now invalidates shared pools correctly, custom creators never receive pool metadata, converged construction diagnostics remain name-neutral, contract-only implementations fail only when using unsupported callback capabilities, and Reverb stays on its safe shared client path.

Stop wrapping the Slack notification router in an object pool because the router owns no external resource and the container already shares the same instance. Direct resolution preserves behavior while eliminating duplicate-factory ownership violations, unnecessary configuration surface, and a proxy whose nullable response return could not be modeled safely by generic forwarding.

Remove HTTP client object pooling and retain connection reuse at the state-owning low-level handler, with a fresh middleware stack and cookie jar for every pending request. Add explicit connection presets and option-layer precedence, canonical reserved-option validation, safe re-registration, request-local retry cookies, custom client and handler overrides, and concurrency coverage proving handler sharing without middleware or cookie leakage.

Describe immutable definitions, canonical fingerprints, normalized options, managed convergence, leases, ownership rules, deterministic closure, maintenance, and explicit proxy surfaces. Rename the public guide to object-pools and distinguish the general-purpose ObjectPool package from Hypervel's internal lower-level connection-pool infrastructure.

Explain client-level S3 and GCS pooling, automatic and explicit identities, lifecycle options, shared-resource purge semantics, and borrow-scoped access to client internals. Document dynamic scoped filesystem decorators, fail-closed prefix behavior, on-demand disk convergence, advanced GCS client configuration, and the relevant differences from Laravel.

Document the named-mailer and MailManager build pooling matrix, default lifecycle options, credential-derived convergence, custom transport safety gates, composite transport behavior, and explicit invalidation. Include a multi-provider tenant example showing how dynamic credentials reuse connections without requiring tenant-specific framework APIs.

Explain queue pool identity, options, purge behavior, job-held leases, terminal release and discard semantics, and capacity sizing for concurrent workers. The guide now makes the Beanstalkd same-connection requirement and SQS lease behavior explicit for operators configuring pooled queue workers.

Replace client-pool documentation with the named connection preset and shared low-level transport-handler model. Document option precedence, request-local middleware and cookies, reserved settings, transport sharing, re-registration behavior, and the distinction between reusable connections and per-request client state.

Explain that the Slack router is a shared stateless dispatcher rather than a pooled external resource. Direct custom channel authors toward the general object-pool API only when their implementation actually owns reusable mutable or network resources.

Add a focused design-register item to verify whether the existing Pusher and Ably broadcaster pools isolate any state that their SDK clients cannot safely share. This preserves the current proven behavior while making the remaining resource-ownership question explicit for a dedicated future review.

coderabbitai · 2026-07-11T08:03:41Z

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: 9f5fc848-5a58-457e-b565-e47141265579

📥 Commits

Reviewing files that changed from the base of the PR and between 5777a9f and 92ec1a4.

📒 Files selected for processing (14)

docs/plans/2026-07-10-object-pool-lifecycle-and-client-pooled-filesystems.md
docs/todo.md
src/coordinator/src/Timer.php
src/filesystem/README.md
src/object-pool/src/Channel.php
src/pool/src/Channel.php
src/pool/src/KeepaliveConnection.php
src/watcher/src/Driver/FindNewerDriver.php
tests/Coordinator/TimerTest.php
tests/ObjectPool/ChannelTest.php
tests/Pool/ChannelTest.php
tests/Pool/Fixtures/KeepaliveConnectionStub.php
tests/Pool/HeartbeatConnectionTest.php
tests/Watcher/Driver/FindNewerDriverTest.php

✅ Files skipped from review due to trivial changes (2)

docs/todo.md
docs/plans/2026-07-10-object-pool-lifecycle-and-client-pooled-filesystems.md

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

tests/Pool/ChannelTest.php
tests/ObjectPool/ChannelTest.php
src/pool/src/Channel.php
src/object-pool/src/Channel.php

📝 Walkthrough

Walkthrough

Hypervel rebuilds pooling around immutable identities, validated options, explicit ownership, leases, idle maintenance, and terminal closure. Filesystem, HTTP, mail, queue, broadcasting, Sentry, watcher, configuration, documentation, and test integrations are updated for these lifecycle contracts.

Changes

Pooling and lifecycle

Layer / File(s)	Summary
Pool identity, ownership, and maintenance `src/object-pool/`, `src/pool/`, `src/database/src/Pool/`, `src/redis/src/Pool/`	Pool definitions and fingerprints replace name-only registration; pools track managed and borrowed resources, support leases, idle trimming, expiry sweeping, discard, and terminal closure.
Pooled consumer integration `src/filesystem/src/`, `src/http/src/Client/`, `src/mail/src/`, `src/queue/src/`, `src/broadcasting/src/`, `src/sentry/src/`	Filesystem clients use per-operation adapter stacks, HTTP connections reuse low-level handlers with request-owned clients and cookies, and other consumers use explicit lease and invalidation paths.
Filesystem scoping and streaming `src/filesystem/src/Scoped*`, `src/filesystem/src/FileResponseBuilder.php`, `src/filesystem/src/LeasedStream.php`	Scoped decorators enforce normalized prefixes and containment, while shared response construction handles ranges, streaming, and lease-backed streams.
Watcher and lifecycle safety `src/watcher/*`, `src/filesystem/src/Filesystem.php`, `src/coordinator/src/Timer.php`	Watcher commands use structured arguments and escaping, subprocesses and reference files have explicit cleanup, temporary replacements clean up on failure, and timer errors use logger or error-log fallback.
Configuration, documentation, and validation `src/boost/docs/`, `src/foundation/config/`, `tests/*`	Documentation, configuration templates, facade annotations, cleanup hooks, and unit/integration coverage are updated for the new contracts.

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

Possibly related PRs

hypervel/components#399: Both changes modify database pool heartbeat and invalidation paths.
hypervel/components#400: Both changes modify database pool connection lifecycle and idle reuse behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.94% 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 is concise and accurately captures the main scope: object-pool lifecycle rebuild plus cloud filesystem pooling 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 refactor/object-pool-and-dynamic-resource-pooling

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

Remove two non-essential documentation changes from this branch so the primary architectural PR remains within external review limits. Exact copies are preserved outside the components repository for a follow-up change after this PR merges.

greptile-apps · 2026-07-11T08:29:18Z

Greptile Summary

This PR is a comprehensive rebuild of the object pool and connection pool lifecycle primitives in Hypervel, replacing the previous ad-hoc approach with explicit ownership tracking (managed/borrowed identity maps), immutable PoolDefinition/PoolFingerprint/PoolOptions value types, and an exactly-once Lease finalization primitive. It also adds cloud filesystem pooling (ClientPooledFilesystem, LeasedStream, ScopedFilesystemProxy), range-aware file streaming (FileResponseBuilder), queue-job pool lease integration, and HTTP client transport sharing.

Pool core redesign: ObjectPool and Pool now track every object by spl_object_id in managed[]/borrowed[] maps; Channel replaces dual-store with an SplQueue + EngineChannel signal for correct cooperative-scheduler semantics; PoolManager.getOrCreate() self-heals closed pools and enforces fingerprint/option equivalence; PoolRecycler replaces ObjectRecycler with idle-pool eviction, sweep, and trim in a single maintenance tick.
Lease primitive: Lease provides exactly-once finalization with a boolean finalized guard; release-callback failures trigger automatic discard; a defensive destructor covers abandoned leases. LeasedStream bridges leases to PHP stream wrappers for transparent filesystem pooling.
Ancillary subsystems: FileResponseBuilder adds Range-header streaming with weak-ETag rejection on If-Range; QueuePoolProxy holds a backend lease post-pop until terminal delete/release/bury; MailManager adds explicit per-transport fingerprint selection with cycle detection for composite transports; HTTP client drops object-pool machinery in favor of shared transport handlers with per-request CookieJar isolation.

Confidence Score: 4/5

This PR is safe to merge; the concurrency invariants are sound and no P0/P1 bugs were found across the ~40 changed files.

Thorough review of all major changed files found no blocking bugs: pool ownership semantics are correct under Swoole's cooperative scheduler, Lease finalization is exactly-once, Channel close correctly wakes all parked waiters, PoolRecycler iteration is safe against concurrent remove(), and DbPool heartbeat uses the correct requeue/destroy paths. The score is 4 rather than 5 only because of the size and complexity of the change — several subsystems (queue lease teardown, composite transport cycle detection, HTTP option-layer merging) interact in ways that warrant careful integration testing before production promotion.

src/queue/src/QueuePoolProxy.php and src/queue/src/Jobs/BeanstalkdJob.php / SqsJob.php — the cascading lease-teardown paths on pop failure and job-requeue fallback are the most intricate new code paths and deserve focused integration testing.

Important Files Changed

Filename	Overview
src/object-pool/src/ObjectPool.php	Rebuilds object pool with explicit ownership tracking (managed/borrowed maps), monotonic deadline arithmetic, lifecycle-safe close(), and single destroy path with capacity signalling. Logic is correct for Swoole's cooperative scheduling model.
src/object-pool/src/Lease.php	Exactly-once lease finalization with defensive destructor; release-callback failure correctly triggers discard instead of a silent return-to-pool. Clean implementation.
src/object-pool/src/Channel.php	Unified idle queue (SplQueue) with a separate coroutine-only signal channel replaces the old dual-store approach. wait()/signal()/close() correctly wakes waiters on pool closure and capacity changes.
src/object-pool/src/PoolManager.php	getOrCreate() self-heals closed pools, performs full definition-equivalence checks (resource type, fingerprint, options), and fails fast on conflicting registrations. remove() correctly deregisters before closing.
src/object-pool/src/PoolFingerprint.php	Type-tagged canonicalization handles null/bool/int/float/string/enum/list/map correctly; map ordering is sort-stable by type+key; throws on un-canonicalizable types rather than silently converging.
src/object-pool/src/PoolOptions.php	Strict option validation (unknown key rejection, integer-only integers, finite-only floats, capacity ordering). idle_ttl defaults to 300s; callers can disable with explicit null. Validated with equals() for pool-definition matching.
src/pool/src/Pool.php	Adopts same ownership model (managedConnections/borrowedConnections maps) as ObjectPool; terminal close(), requeueConnection(), destroyConnection() mirror the upper pool. checkIdleConnection() replaces flushOne() with explicit health-check discards.
src/pool/src/Channel.php	Kept in sync with object-pool Channel; same SplQueue + signal-channel design. Comment notes the sync requirement.
src/filesystem/src/FileResponseBuilder.php	New file handling range-aware streaming; weak-ETag If-Range and empty-read-loop issues are confirmed fixed. Range validation, suffix-range, and open-ended range all handled. Stream closed in finally under both success and failure paths.
src/filesystem/src/LeasedStream.php	PHP stream-wrapper that holds a pool Lease until stream close; $registered is correctly process-global (single registration per worker). finalize() is idempotent via null-setting both inner and lease before cleanup.
src/filesystem/src/ScopedFilesystemProxy.php	Comprehensive path-boundary enforcement via Flysystem's WhitespacePathNormalizer (handles '..' collapse); fails closed on empty prefix; rejects unknown methods to prevent scope bypass; stripPrefix validates returned paths stay inside scope.
src/filesystem/src/FilesystemPoolProxy.php	Refactored to use InteractsWithPooledFilesystem trait; generic __call() removed; callbacks applied per-borrow in configureBorrowed(); lease-aware response streaming via buildFileResponse().
src/queue/src/QueuePoolProxy.php	pop() holds a lease until job terminal operation; cascading error handling (discard→report→throw) correctly handles nested failures from withPoolLease and job.release(). getConnectionName() returns proxy-held state rather than borrowing.
src/queue/src/Jobs/Job.php	Adds withPoolLease()/releasePoolLease()/discardPoolLease() lifecycle to the base Job class; poolLeaseIsFinalized() enables subclasses to guard access after terminal operations. discardPoolLeaseAfterFailure() preserves primary exceptions.
src/queue/src/Jobs/BeanstalkdJob.php	Terminal operations (release/bury/delete) correctly call discardPoolLeaseAfterFailure on backend error and releasePoolLease on success; onPoolLeaseAttached() pre-caches attempt count so it's readable after finalization.
src/mail/src/MailManager.php	transportConstructionConfig() explicitly resolves construction inputs per transport type, enabling correct fingerprinting; composite transports detect cycles and recursively fingerprint children; on-demand mailers can now opt into pooling via pool key.
src/http/src/Client/Factory.php	Removes object-pool-based HTTP client; registered connections now share a low-level transport handler (createConnectionHandler); per-request middleware stacks and cookie jars are isolated in PendingRequest.
src/http/src/Client/PendingRequest.php	Cookies moved to a request-owned CookieJar; option layers (base → connection → request → per-call) merged via mergeOptionLayers(); reserved options rejected at each boundary. getOptions() now reflects the full merged stack.
src/database/src/Pool/DbPool.php	close() now calls parent::close() after clearing the heartbeat; heartbeatConnection uses requeueConnection instead of release (connection not formally borrowed); discardHeartbeatConnection delegates to destroyConnection correctly.
src/object-pool/src/PoolRecycler.php	Replaces ObjectRecycler; maintainPools() evicts idle pools before sweeping/trimming live ones; pool identity passed to remove() ensures only the expected instance is closed; no maintenance update of activity timestamps (correct).

_{Reviews (3): Last reviewed commit: "Document filesystem architecture follow-..." | Re-trigger Greptile}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (5)

src/queue/src/QueuePoolProxy.php (1)
152-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: document the defensive inner discard().

This nested $lease->discard() inside the recovery catch is a safe idempotent no-op whenever the job's own terminal release() already finalized the lease (the common case), and only matters if withPoolLease() fails before actually attaching the lease. A one-line comment explaining that edge case would help future readers, since it's not obvious why a discard follows a call that (in the common path) already finalized the lease itself.
🤖 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/queue/src/QueuePoolProxy.php` around lines 152 - 168, In the recovery
path around withPoolLease(), add a concise comment immediately before
$lease->discard() explaining that release(0) usually finalizes the lease, while
discard() safely handles failures that occur before the lease is attached.
src/broadcasting/src/BroadcastManager.php (1)
280-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the duplicated pool-definition construction.

Arr::except($config, ['pool']) followed by poolDefinition($config['driver'], $config['pool'] ?? [], $constructionConfig) is repeated identically in resolve() and purge(). Consider a small private helper (e.g. poolDefinitionForConfig(array $config): array{PoolDefinition, array} or similar) to keep the two call sites from drifting.
♻️ Suggested consolidation
+    /**
+     * Build the construction config and pool definition for a broadcaster config.
+     *
+     * `@return` array{0: array, 1: PoolDefinition}
+     */
+    protected function poolDefinitionForConfig(array $config): array
+    {
+        $constructionConfig = Arr::except($config, ['pool']);
+
+        return [
+            $constructionConfig,
+            $this->poolDefinition($config['driver'], $config['pool'] ?? [], $constructionConfig),
+        ];
+    }
Also applies to: 494-501
🤖 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/broadcasting/src/BroadcastManager.php` around lines 280 - 289, Extract
the repeated pool setup from resolve() and purge() into a private helper, such
as poolDefinitionForConfig(), that derives constructionConfig with Arr::except
and builds the pool definition using the driver and optional pool configuration.
Update both call sites to reuse the helper while preserving the existing
createPoolProxy and doResolve behavior.
src/http/src/Client/PendingRequest.php (1)
1834-1846: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the two-step layer-merge semantics.

mergeOptionLayers() relies on a subtle interaction: array_merge_recursive accumulates mergeable keys (e.g. headers) across layers, then array_replace_recursive against the raw layer overrides same-key values rather than duplicating them. This correctly avoids turning connection-level headers into multi-value arrays when a later layer redefines the same header, but the reasoning isn't obvious from the code. A short inline comment explaining why both calls are needed (and that same-key values override across layers while distinct keys accumulate) would help future maintainers avoid "simplifying" this into a bug.
🤖 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/http/src/Client/PendingRequest.php` around lines 1834 - 1846, Add a
concise inline comment in mergeOptionLayers() documenting that
array_merge_recursive accumulates distinct mergeable options across layers,
while array_replace_recursive applies the raw layer so later same-key values
override instead of becoming duplicated arrays; preserve the existing two-step
merge behavior.
tests/Mail/MailManagerTest.php (1)
559-604: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor duplication between the two purge/forget tests.

testPurgeInvalidatesACachedTransportPool and testForgetIsCacheOnlyAndUncachedPurgeDerivesThePoolIdentity repeat the same mail.mailers.smtp setup and pool-lookup boilerplate. Could extract a small helper, but this is optional given the tests are otherwise clear and independent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/Mail/MailManagerTest.php` around lines 559 - 604, Optionally extract
the repeated SMTP configuration, MailManager/transport creation, PoolFactory
lookup, and pool identity setup from testPurgeInvalidatesACachedTransportPool
and testForgetIsCacheOnlyAndUncachedPurgeDerivesThePoolIdentity into a small
private test helper, then reuse it while keeping both tests independent and
behavior unchanged.
src/filesystem/src/FilesystemManager.php (1)
763-769: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a named default for cached static state.

Define DEFAULT_S3_ARGUMENT_NAMES, initialize $s3ArgumentNames from it, and reset to that constant in flushState().

As per coding guidelines, src/**/src/**.php: “reset static defaults using DEFAULT_* class constants when the initial value and reset value match.”
🤖 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/filesystem/src/FilesystemManager.php` around lines 763 - 769, Define a
DEFAULT_S3_ARGUMENT_NAMES class constant in FilesystemManager, initialize the
static $s3ArgumentNames property from that constant, and update flushState() to
reset it to the same constant instead of null.
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/filesystem/src/FileResponseBuilder.php`:
- Around line 74-96: Update build() to detect HEAD requests before registering
or executing the streaming callback, returning a response with the existing
headers and status but no body. Preserve the current streaming behavior for
non-HEAD requests, including resolver invocation and stream cleanup.

---

Nitpick comments:
In `@src/broadcasting/src/BroadcastManager.php`:
- Around line 280-289: Extract the repeated pool setup from resolve() and
purge() into a private helper, such as poolDefinitionForConfig(), that derives
constructionConfig with Arr::except and builds the pool definition using the
driver and optional pool configuration. Update both call sites to reuse the
helper while preserving the existing createPoolProxy and doResolve behavior.

In `@src/filesystem/src/FilesystemManager.php`:
- Around line 763-769: Define a DEFAULT_S3_ARGUMENT_NAMES class constant in
FilesystemManager, initialize the static $s3ArgumentNames property from that
constant, and update flushState() to reset it to the same constant instead of
null.

In `@src/http/src/Client/PendingRequest.php`:
- Around line 1834-1846: Add a concise inline comment in mergeOptionLayers()
documenting that array_merge_recursive accumulates distinct mergeable options
across layers, while array_replace_recursive applies the raw layer so later
same-key values override instead of becoming duplicated arrays; preserve the
existing two-step merge behavior.

In `@src/queue/src/QueuePoolProxy.php`:
- Around line 152-168: In the recovery path around withPoolLease(), add a
concise comment immediately before $lease->discard() explaining that release(0)
usually finalizes the lease, while discard() safely handles failures that occur
before the lease is attached.

In `@tests/Mail/MailManagerTest.php`:
- Around line 559-604: Optionally extract the repeated SMTP configuration,
MailManager/transport creation, PoolFactory lookup, and pool identity setup from
testPurgeInvalidatesACachedTransportPool and
testForgetIsCacheOnlyAndUncachedPurgeDerivesThePoolIdentity into a small private
test helper, then reuse it while keeping both tests independent and behavior
unchanged.

🪄 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: a8f469e1-aa8d-4d43-aba6-d63d16763514

📥 Commits

Reviewing files that changed from the base of the PR and between d9a4606 and 0bcc2ec.

📒 Files selected for processing (150)

docs/plans/2026-07-10-object-pool-lifecycle-and-client-pooled-filesystems.md
src/boost/docs-ported.md
src/boost/docs/filesystem.md
src/boost/docs/http-client.md
src/boost/docs/mail.md
src/boost/docs/notifications.md
src/boost/docs/object-pools.md
src/boost/docs/queues.md
src/broadcasting/src/BroadcastManager.php
src/broadcasting/src/BroadcastPoolProxy.php
src/broadcasting/src/Broadcasters/Broadcaster.php
src/contracts/src/Pool/PoolInterface.php
src/database/src/Pool/DbPool.php
src/database/src/Pool/PoolFactory.php
src/database/src/Pool/PooledConnection.php
src/engine/src/Channel.php
src/filesystem/src/AwsS3V3Adapter.php
src/filesystem/src/ClientPooledFilesystem.php
src/filesystem/src/Concerns/InteractsWithPooledFilesystem.php
src/filesystem/src/FileResponseBuilder.php
src/filesystem/src/FilesystemAdapter.php
src/filesystem/src/FilesystemManager.php
src/filesystem/src/FilesystemPoolProxy.php
src/filesystem/src/GoogleCloudStorageAdapter.php
src/filesystem/src/LeasedStream.php
src/filesystem/src/LocalFilesystemAdapter.php
src/filesystem/src/ScopedCloudFilesystemProxy.php
src/filesystem/src/ScopedFilesystemProxy.php
src/foundation/config/broadcasting.php
src/foundation/config/filesystems.php
src/foundation/config/queue.php
src/foundation/src/Testing/Concerns/InteractsWithTestCaseLifecycle.php
src/http/src/Client/ClientPoolProxy.php
src/http/src/Client/Factory.php
src/http/src/Client/PendingRequest.php
src/http/src/Client/ReservedOptions.php
src/mail/src/MailManager.php
src/mail/src/TransportPoolProxy.php
src/notifications/src/ChannelManager.php
src/notifications/src/NotificationPoolProxy.php
src/object-pool/README.md
src/object-pool/src/Channel.php
src/object-pool/src/Contracts/Factory.php
src/object-pool/src/Contracts/ObjectPool.php
src/object-pool/src/Contracts/RecycleStrategy.php
src/object-pool/src/Contracts/Recycler.php
src/object-pool/src/Contracts/TimeStrategy.php
src/object-pool/src/Lease.php
src/object-pool/src/ObjectPool.php
src/object-pool/src/ObjectPoolServiceProvider.php
src/object-pool/src/ObjectRecycler.php
src/object-pool/src/PoolDefinition.php
src/object-pool/src/PoolErrorReporter.php
src/object-pool/src/PoolFingerprint.php
src/object-pool/src/PoolManager.php
src/object-pool/src/PoolOption.php
src/object-pool/src/PoolOptions.php
src/object-pool/src/PoolProxy.php
src/object-pool/src/PoolRecycler.php
src/object-pool/src/SimpleObjectPool.php
src/object-pool/src/Strategies/TimeStrategy.php
src/object-pool/src/Traits/HasPoolProxy.php
src/pool/src/Channel.php
src/pool/src/Connection.php
src/pool/src/ConstantFrequency.php
src/pool/src/Pool.php
src/pool/src/PoolOption.php
src/queue/src/Jobs/BeanstalkdJob.php
src/queue/src/Jobs/DatabaseJob.php
src/queue/src/Jobs/FakeJob.php
src/queue/src/Jobs/Job.php
src/queue/src/Jobs/RedisJob.php
src/queue/src/Jobs/SqsJob.php
src/queue/src/Jobs/SyncJob.php
src/queue/src/QueueManager.php
src/queue/src/QueuePoolProxy.php
src/queue/src/SyncQueue.php
src/redis/src/Pool/PoolFactory.php
src/redis/src/Pool/RedisPool.php
src/redis/src/RedisConnection.php
src/sentry/config/sentry.php
src/sentry/src/SentryServiceProvider.php
src/sentry/src/Transport/HttpPoolTransport.php
src/sentry/src/Transport/Pool.php
src/support/src/Facades/Broadcast.php
src/support/src/Facades/Http.php
src/support/src/Facades/Mail.php
src/support/src/Facades/Notification.php
src/support/src/Facades/Queue.php
src/support/src/Facades/Storage.php
src/testing/src/PHPUnit/AfterEachTestSubscriber.php
tests/Broadcasting/BroadcastPoolProxyTest.php
tests/Database/PoolFactoryTest.php
tests/Filesystem/AwsS3V3AdapterTest.php
tests/Filesystem/ClientPooledFilesystemTest.php
tests/Filesystem/FileResponseBuilderTest.php
tests/Filesystem/FilesystemAdapterTest.php
tests/Filesystem/FilesystemManagerTest.php
tests/Filesystem/FilesystemPoolProxyTest.php
tests/Filesystem/GoogleCloudStorageAdapterTest.php
tests/Filesystem/LeasedStreamTest.php
tests/Filesystem/ScopedFilesystemProxyTest.php
tests/Http/HttpClientTest.php
tests/Http/HttpConnectionTest.php
tests/Integration/Broadcasting/BroadcastManagerTest.php
tests/Integration/Cache/CacheFunnelTestCase.php
tests/Integration/Database/PooledConnectionTest.php
tests/Integration/Database/Sqlite/DbPoolHeartbeatTest.php
tests/Integration/Database/Sqlite/InMemorySqliteSharedPdoTest.php
tests/Integration/Database/Sqlite/PoolConnectionManagementTest.php
tests/Integration/Database/Sqlite/QueryDurationThresholdPooledTest.php
tests/Integration/Engine/HttpClientConnectionTest.php
tests/Integration/Horizon/Feature/Listeners/StoreTagsForFailedTest.php
tests/Integration/Horizon/IntegrationTestCase.php
tests/Mail/MailManagerTest.php
tests/Mail/MailSesV2TransportTest.php
tests/Notifications/NotificationChannelManagerTest.php
tests/ObjectPool/ChannelTest.php
tests/ObjectPool/Fixtures/FooPool.php
tests/ObjectPool/HasPoolProxyTest.php
tests/ObjectPool/LeaseTest.php
tests/ObjectPool/ObjectPoolTest.php
tests/ObjectPool/ObjectRecyclerTest.php
tests/ObjectPool/PoolDefinitionTest.php
tests/ObjectPool/PoolErrorReporterTest.php
tests/ObjectPool/PoolFingerprintTest.php
tests/ObjectPool/PoolManagerTest.php
tests/ObjectPool/PoolOptionsTest.php
tests/ObjectPool/PoolProxyTest.php
tests/ObjectPool/PoolRecyclerTest.php
tests/ObjectPool/SimpleObjectPoolTest.php
tests/ObjectPool/TimeStrategyTest.php
tests/Pool/ChannelTest.php
tests/Pool/ConnectionTest.php
tests/Pool/FrequencyTest.php
tests/Pool/PoolNonCoroutineTest.php
tests/Pool/PoolOptionTest.php
tests/Pool/PoolTest.php
tests/Queue/FakeJobTest.php
tests/Queue/PooledJobWorkerTest.php
tests/Queue/QueueBeanstalkdJobTest.php
tests/Queue/QueueManagerTest.php
tests/Queue/QueuePoolProxyTest.php
tests/Queue/QueueSqsJobTest.php
tests/Redis/PoolFactoryTest.php
tests/Redis/RedisConnectionTest.php
tests/Redis/RedisPoolHeartbeatTest.php
tests/Sentry/ConfigTest.php
tests/Sentry/FlushLifecycleTest.php
tests/Sentry/HttpPoolTransportTest.php

💤 Files with no reviewable changes (15)

tests/ObjectPool/Fixtures/FooPool.php
src/notifications/src/NotificationPoolProxy.php
src/object-pool/src/Strategies/TimeStrategy.php
src/object-pool/src/Contracts/RecycleStrategy.php
src/http/src/Client/ClientPoolProxy.php
src/object-pool/src/Contracts/TimeStrategy.php
src/object-pool/src/ObjectRecycler.php
tests/ObjectPool/TimeStrategyTest.php
tests/ObjectPool/ObjectRecyclerTest.php
src/sentry/config/sentry.php
src/object-pool/src/PoolOption.php
src/pool/src/Connection.php
src/redis/src/RedisConnection.php
src/database/src/Pool/PooledConnection.php
src/notifications/src/ChannelManager.php

Enforce strong If-Range entity-tag comparison, preserve HEAD body suppression, and reject empty non-EOF reads instead of allowing response streams to spin indefinitely. Also fail fast when a non-seekable ranged stream makes no positioning progress, and add regressions for weak validators, truncated ranges, stalled streams, resource closure, and HEAD response emission.

Launch watcher-owned subprocesses with argv arrays so paths and arguments remain literal and process termination targets the real child. Escape every path that must still cross the find drivers' shell boundary. Give fswatch deterministic process and pipe ownership, make driver shutdown explicit and idempotent, and distinguish child exit from read failure without hot spinning. Replace FindNewer's shared predictable anchors with per-driver atomic reference files. Defer cleanup across yielding scans, reject unsafe immediate restarts, and remove every owned file without throwing from destructor paths. Document the list-shaped server command configuration and cover process cleanup, shell metacharacters, reference ownership, stop races, and failure diagnostics.

Turn Filesystem::replace() into a checked write, chmod, and rename transaction. Exact byte-count validation prevents partial content from replacing a valid target, while write-before-chmod supports restrictive final modes and keeps incomplete data private. Normalize tempnam warnings at cache and Testbench boundaries so framework-owned diagnostics remain reachable. Build temporary Blade views through a private checked write followed by an atomic rename, with primary-preserving cleanup on every failure. Add regressions for restrictive modes, missing and unwritable destinations, fallback containment, and temporary-file cleanup.

Explain the layered HTTP option merge semantics and the queue lease discard backstop where the local code is otherwise easy to misread during future maintenance. Document that nullable lazy caches and callback slots use null as a structural sentinel, so static-state cleanup should not manufacture DEFAULT constants for values that are not configurable defaults.

Record the final filesystem response, watcher lifecycle, shell-boundary, reference-file ownership, and transactional temporary-file decisions as implemented. Keep the bug inventory, design rationale, regression matrix, and implementation order aligned with the finished code rather than preserving discarded intermediate ideas.

binaryfire · 2026-07-11T11:16:00Z

@greptileai @coderabbitai Thanks. I audited every finding against the implementation and pushed the follow-up as five focused commits.

Resolved:

If-Range now uses strong comparison on both sides. Weak validators return the full 200 response.
Empty non-EOF reads now fail immediately and close the stream. I intentionally did not add a retry counter: a blocking filesystem stream returning no progress is an error, and an arbitrary cap only delays it. The sibling non-seekable range-positioning loop received the same fix.
HEAD behavior is pinned at the existing response boundary. withoutBody() prevents callback and resolver execution while preserving headers and status and ignoring Range.
Added the requested WHY comments for layered HTTP option merging and the queue lease discard backstop.

Reviewed but intentionally unchanged:

The broadcast pool-definition extraction would replace two short call sites with a helper returning a coupled construction-config and definition tuple. The current code is clearer.
The mail purge test duplication is small and keeps the two lifecycle cases independent and readable.
DEFAULT_S3_ARGUMENT_NAMES would misdescribe null as configurable data. Null is the structural lazy-cache sentinel. AGENTS.md now states that exception explicitly.

The same-family audit also found and fixed issues beyond the review lines: non-seekable range stalls, fswatch process and pipe leaks and hot spinning, shell-interpreted watcher commands and unescaped find paths, shared and leaked FindNewer reference files including a stop-during-yield race, unchecked tempnam diagnostics, partial atomic replacements, and partial temporary Blade writes.

The main review summaries contained no other actionable nitpicks or outside-diff findings.

Verification is green from the exact pushed state: formatting, PHPStan, the full parallel suite, Testbench and integration checks, and dogfood. A second full code review found no remaining issues.

Commits: 09e1143, e1e5fc6, 18d106c, d9eb4d7, 5777a9f.

greptile-apps · 2026-07-11T11:16:27Z

Thanks for the thorough follow-up. The resolutions are all sound:

Strong If-Range comparison is the correct fix — weak validators producing a 206 would be a spec violation.
Failing immediately on empty non-EOF reads rather than adding a retry cap is the right call. A stalled blocking stream is definitively broken; a retry counter would just defer the failure.
HEAD pinning at the response boundary with withoutBody() is clean.
The DEFAULT_S3_ARGUMENT_NAMES note in AGENTS.md is the right place to document the null sentinel distinction — it would be genuinely misleading to expose that as a named constant.

The additional audit findings (fswatch leaks, shell-interpreted watcher commands, unescaped find paths, FindNewer reference file races, tempnam diagnostics, partial atomic writes) are the kind of thing that's easy to miss when reviewing against a specific diff boundary. Good catch on those.

Everything looks good. Happy to approve.

coderabbitai · 2026-07-11T11:16:49Z

Tip

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

@binaryfire Thanks for the thorough follow-up and for auditing every finding individually rather than applying blanket fixes. The resolutions all sound correct to me:

Strong If-Range comparison on both sides (falling back to full 200 for weak validators) is the correct RFC-aligned behavior.
Treating an empty, non-EOF read as an immediate hard error rather than retrying is the right call for a blocking stream — an arbitrary retry cap would just mask a real problem behind added latency, and applying the same fix to the non-seekable range-positioning loop closes that gap consistently.
Pinning HEAD handling at the existing response boundary via withoutBody() (skipping callback/resolver execution while preserving status/headers and ignoring Range) is a clean way to avoid unnecessary work on responses that will discard the body anyway.
Appreciate the added WHY comments on the HTTP option-layering precedence and the queue lease discard backstop — those are exactly the kind of non-obvious invariants worth documenting inline.

On the intentionally-unchanged items, your reasoning holds up:

Keeping the two BroadcastManager call sites as-is rather than introducing a helper that returns a coupled construction-config/definition tuple is a reasonable readability trade-off.
Independent mail purge tests for the cache-only vs. pool-invalidation lifecycle cases are worth the small duplication.
Documenting in AGENTS.md that null in the cached S3 SDK argument list is a structural lazy-cache sentinel (not configurable data) is a good clarification to prevent future confusion.

Thanks also for flagging and fixing the additional same-family issues found during the audit (fswatch leaks/spinning, unescaped find paths, FindNewer races, tempnam diagnostics, atomic/Blade write partial-write issues) — those are exactly the kind of latent bugs that a systematic audit like this surfaces.

Given formatting, PHPStan, the full parallel suite, Testbench/integration checks, and dogfood are all green from the pushed state, this looks ready from my side.

(=^-ω-^=)

✏️ Learnings added

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

Timestamp: 2026-07-11T11:16:48.162Z
Learning: In src/filesystem/src/FileResponseBuilder.php, an empty non-EOF stream read is treated as an immediate hard error (closing the stream) rather than being retried with a counter, because a blocking filesystem stream returning no progress is considered a genuine error and an arbitrary retry cap would only delay failure. The same fix is applied to the sibling non-seekable range-positioning loop.

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

Timestamp: 2026-07-11T11:16:48.162Z
Learning: In src/broadcasting/src/BroadcastManager.php, the two short call sites that build pool-definition construction config are intentionally kept separate rather than extracted into a shared helper, since a helper returning a coupled construction-config/definition tuple was judged less clear than the current code.

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

Timestamp: 2026-07-11T11:16:48.162Z
Learning: In tests/Mail/MailManagerTest.php, some duplication between the cache-only invalidation test and the pool-invalidation test for mailer purge behavior is intentional, to keep the two lifecycle scenarios independent and readable rather than merging them.

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

Timestamp: 2026-07-11T11:16:48.162Z
Learning: In src/filesystem/src/FilesystemManager.php, the cached S3 SDK argument-name list (e.g., DEFAULT_S3_ARGUMENT_NAMES) uses `null` as a structural lazy-cache-not-yet-populated sentinel, not as configurable/user data. This distinction is documented explicitly in AGENTS.md.

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.

coderabbitai

Actionable comments posted: 1

🤖 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/watcher/src/Driver/FindNewerDriver.php`:
- Around line 52-65: Update the scanning flow in the relevant method to record
the next cutoff before calling scan(): touch the alternate reference file first,
then run scan(), and advance the scan index only after scanning completes.
Preserve the existing changed-file handling while ensuring changes occurring
during the scan remain eligible for the next scan.

🪄 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: b473db39-2119-4017-8234-8b89266a8d90

📥 Commits

Reviewing files that changed from the base of the PR and between 0bcc2ec and 5777a9f.

📒 Files selected for processing (26)

AGENTS.md
docs/plans/2026-07-10-object-pool-lifecycle-and-client-pooled-filesystems.md
src/boost/docs/mail.md
src/filesystem/src/FileResponseBuilder.php
src/filesystem/src/Filesystem.php
src/filesystem/src/FilesystemAdapter.php
src/foundation/src/Console/ConfigCacheCommand.php
src/foundation/src/Console/RouteCacheCommand.php
src/foundation/src/Testing/Concerns/InteractsWithViews.php
src/http/src/Client/PendingRequest.php
src/queue/src/QueuePoolProxy.php
src/watcher/README.md
src/watcher/config/watcher.php
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/ServerRestartStrategy.php
tests/Filesystem/FileResponseBuilderTest.php
tests/Filesystem/FilesystemAdapterTest.php
tests/Filesystem/FilesystemTest.php
tests/Watcher/Driver/FindDriverTest.php
tests/Watcher/Driver/FindNewerDriverTest.php
tests/Watcher/Driver/FswatchDriverTest.php
tests/Watcher/ServerRestartStrategyTest.php

✅ Files skipped from review due to trivial changes (4)

AGENTS.md
src/watcher/README.md
src/watcher/src/Driver/DriverInterface.php
docs/plans/2026-07-10-object-pool-lifecycle-and-client-pooled-filesystems.md

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

src/boost/docs/mail.md
src/queue/src/QueuePoolProxy.php
src/filesystem/src/FilesystemAdapter.php
src/filesystem/src/FileResponseBuilder.php
tests/Filesystem/FilesystemAdapterTest.php
src/http/src/Client/PendingRequest.php

Record the alternate reference timestamp before each scan and rotate reference roles after every successful pass, including quiet scans. This preserves at-least-once detection for files changed after find has already passed their path instead of advancing both cutoffs beyond the change. Observe stop requests after both overridable I/O boundaries so shutdown during a reference update skips the scan, while shutdown during a scan skips rotation and publication. Add deterministic logical-clock and channel-driven regressions for late changes, quiet rotation, scan recovery, and both shutdown windows.

Make the object-pool and connection-pool channel wrappers enforce their terminal lifecycle directly. A push after close now returns false without retaining data in the local queue, matching the boolean API and engine-channel failure semantics. Keep both package implementations synchronized and add mirrored regressions proving rejected values are neither counted nor retrievable after closure.

Preserve recurring Timer execution after callback failures while ensuring the failure is visible. Timer now reports through its configured logger and falls back to the PHP error log when no logger is available; one-shot timer behavior remains unchanged. Apply the same logger-or-error-log rule to heartbeat failures caught inside KeepaliveConnection after clearing the failed connection. Add regressions for both Timer reporting routes, continued execution after failure, and logger-less heartbeat cleanup and diagnostics.

Record the final find-newer cutoff rotation, closed-channel rejection, recurring timer reporting, and keepalive fallback behavior as implemented. Keep the verified bug inventory, lifecycle rationale, regression matrix, and implementation order aligned with the finished code, including both watcher shutdown windows and the deliberate Timer::after distinction.

Describe Hypervel's client-pooled cloud filesystem design, borrow-scoped raw access, and dynamic scoped filesystem decorators in the component README. Record the remaining owner review of Pusher and Ably broadcaster state while preserving their current pooling behavior.

binaryfire · 2026-07-11T12:17:40Z

@greptileai @coderabbitai Follow-up fixes are pushed.

CodeRabbit cutoff finding:

Fixed in 3dd76ec. FindNewer now records the alternate cutoff before scanning, scans against the previous cutoff, and rotates after every successful scan, including quiet scans. This preserves at-least-once detection for changes made after find has passed their path. The regression models the lost-change timeline without timing assumptions.

Greptile observations:

Accepted the closed-channel point. Fixed in 7844d31 for both object-pool and connection-pool channels. push() now returns false after close without retaining the object. Pool callers need no new branch because their synchronous closed guards make this path unreachable internally; the channel now enforces its own lifecycle contract.
Kept PoolProxy::invoke() unchanged. The apparent asymmetry is deliberate exception precedence: when an operation succeeds, a release failure is the only failure and propagates; when both fail, the operation remains primary and the cleanup failure is reported. Direct regressions cover both paths.

The same-family audit found and fixed three additional issues:

A stop request arriving during the new pre-scan reference update could still start an expensive scan. FindNewer now checks stop after both overridable I/O boundaries, with channel-driven regressions for both shutdown windows.
Recurring Timer callback failures disappeared when no logger was configured. Fixed in dcb3917: tick() uses the configured logger or falls back to error_log while preserving the next tick. One-shot after() behavior remains fail-fast and unchanged.
KeepaliveConnection caught heartbeat failures below the Timer boundary but silently discarded them without a logger. It now clears failed connection state first, then reports through the logger or error_log fallback.

The implementation plan and ancillary component documentation were updated in 5ff105f and 92ec1a4.

Validation is green from the pushed state: formatting, PHPStan, the complete parallel suite, Testbench and integration checks, and dogfood. A fresh full code review completed with zero findings.

coderabbitai · 2026-07-11T12:18:08Z

Tip

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

@binaryfire Thanks for the detailed write-up on this second round — the fixes and reasoning all check out:

The cutoff-rotation fix for FindNewer (recording the alternate cutoff before scanning, scanning against the previous cutoff, rotating after every scan including quiet ones) correctly closes the lost-change window without relying on timing assumptions — that's the right way to guarantee at-least-once detection here.
Making push() return false after close() on both channel implementations, without retaining the object, is the correct lifecycle contract for the channel to own itself. Good call confirming the pool callers don't need new branches since their synchronous closed-guards already make that path unreachable.
Keeping PoolProxy::invoke() as-is makes sense — the exception precedence you described (release failure propagates alone on success; operation failure stays primary with cleanup failure reported when both fail) is a deliberate and correct asymmetry, not an oversight.
The three additional same-family fixes are good catches: closing the stop-during-pre-scan-update window in FindNewer, restoring visibility into recurring Timer callback failures via logger/error_log fallback, and ensuring KeepaliveConnection heartbeat failures are reported rather than silently discarded.

Appreciate the continued rigor — green formatting, PHPStan, full parallel suite, Testbench/integration checks, dogfood, and a fresh zero-finding review gives strong confidence in this state.

(=^･ω･^=)

✏️ Learnings added

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

Timestamp: 2026-07-11T12:18:06.950Z
Learning: In src/watcher/src/Driver/FindNewerDriver.php, FindNewer uses two alternating reference/cutoff files: the driver records the alternate cutoff before scanning, scans against the previous cutoff, and rotates cutoffs after every successful scan (including quiet scans with no changes). This preserves at-least-once change detection for files modified after `find` has already passed their path during a scan. FindNewer also checks for a pending stop request after both overridable I/O boundaries (recording the pre-scan cutoff and performing the scan) to avoid starting an expensive scan when a stop was requested during the pre-scan update.

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

Timestamp: 2026-07-11T12:18:06.950Z
Learning: In the object-pool and connection-pool Channel implementations (src/object-pool/src/Channel.php and src/pool/src/Channel.php), `push()` now returns `false` after `close()` without retaining the pushed object, since pool callers already have synchronous closed-guards that make this path unreachable internally, but the channel itself needs to enforce its own lifecycle contract independently.

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

Timestamp: 2026-07-11T12:18:06.950Z
Learning: In src/object-pool/src/PoolProxy.php, the exception handling asymmetry in `invoke()` is intentional: when the wrapped operation succeeds but the release/cleanup fails, the release failure propagates as the only failure; when both the operation and the cleanup fail, the operation's exception remains primary and the cleanup failure is only reported (not thrown), so the original failure is not masked.

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

Timestamp: 2026-07-11T12:18:06.950Z
Learning: Hypervel's Timer abstraction now distinguishes recurring `tick()` failures from one-shot `after()` failures: `tick()` callback failures are reported via the configured logger (falling back to `error_log` when no logger is configured) while preserving the next scheduled tick, whereas `after()` remains fail-fast and unchanged. Similarly, `KeepaliveConnection` heartbeat failures caught below the Timer boundary now clear the failed connection state first, then report the failure through the logger or `error_log` fallback instead of silently discarding it.

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.

greptile-apps · 2026-07-11T12:18:29Z

All three batches look correct from the commit details.

7844d31 — Both Channel implementations now enforce their own terminal lifecycle; push() after close discards the value and returns false without incrementing counts. Mirrored regressions in both packages confirm neither the count nor the value is observable post-closure. The pool callers' existing synchronous closed guards make this path unreachable in normal operation, so there's no new branch burden on consumers — the channel just stops being a footgun if a pool boundary is ever bypassed.

3dd76ec — Recording the alternate reference before the scan and rotating after every successful pass (including quiet ones) is the right fix for the at-least-once window. The two shutdown checkpoints — after the reference update and after the scan — cover both TOCTOU windows cleanly. The logical-clock regressions are the right tool here since they don't carry timing assumptions.

dcb3917 — Timer recurring path now reports through the configured logger with error_log fallback and preserves the next tick. KeepaliveConnection clears the failed connection before reporting, which is the correct ordering (clear-then-log rather than log-then-clear avoids a second heartbeat racing on the stale connection during the log write). The regression coverage across both logger-present and logger-absent paths is thorough.

The audit catches (stop-during-reference-update, silent timer failures, silent heartbeat failures) are exactly the class of issue that surfaces in long-lived workers under load but is hard to reproduce in short test runs — good to have all three nailed with deterministic regressions rather than timing-sensitive ones.

Nothing outstanding from my side. The PR is in good shape.

binaryfire added 30 commits July 11, 2026 07:49

binaryfire added 2 commits July 11, 2026 08:11

chore: defer ancillary documentation updates

0bcc2ec

Remove two non-essential documentation changes from this branch so the primary architectural PR remains within external review limits. Exact copies are preserved outside the components repository for a follow-up change after this PR merges.

Update documentation

10d7340

greptile-apps Bot reviewed Jul 11, 2026

View reviewed changes

Comment thread src/filesystem/src/FileResponseBuilder.php Outdated

Comment thread src/filesystem/src/FileResponseBuilder.php

coderabbitai Bot reviewed Jul 11, 2026

View reviewed changes

Comment thread src/filesystem/src/FileResponseBuilder.php

binaryfire added 5 commits July 11, 2026 11:12

coderabbitai Bot reviewed Jul 11, 2026

View reviewed changes

Comment thread src/watcher/src/Driver/FindNewerDriver.php Outdated

binaryfire added 5 commits July 11, 2026 12:09

binaryfire merged commit 76c175b into 0.4 Jul 11, 2026
36 checks passed

This was referenced Jul 19, 2026

Harden signal and pool resource lifecycles #441

Merged

Harden process, server process, and filesystem lifecycles #442

Merged

binaryfire deleted the refactor/object-pool-and-dynamic-resource-pooling branch July 21, 2026 09:08

coderabbitai Bot mentioned this pull request Jul 23, 2026

fix: harden WebSocket transport and watcher process lifecycles #451

Merged

This was referenced Aug 3, 2026

Complete Broadcasting correctness and SDK lifecycles #469

Merged

Complete Mail correctness, current parity, and package boundaries #470

Merged

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Summary

Background

Pool model

Definitions and identity

Ownership and capacity

Leases and deferred results

Lifecycle and maintenance

Connection pools

Cloud filesystems

Dynamic scoped filesystems

Consumer migrations

Mail

Queues

Broadcasting

Notifications

HTTP

Sentry

API and configuration notes

Performance characteristics

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 11, 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!

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

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

binaryfire commented Jul 11, 2026

Uh oh!

greptile-apps Bot commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

binaryfire commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026

Uh oh!

greptile-apps Bot commented Jul 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

binaryfire commented Jul 11, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

greptile-apps Bot commented Jul 11, 2026 •

edited

Loading