Harden signal and pool resource lifecycles - #441
Merged
Conversation
Replace timeout polling and reversible stopped state with terminal manager-owned teardown. SignalManager now tracks only coroutines parked in native signal waits and exception-cancels those exact waiters during process exit, while allowing active handlers to finish normally. Preserve transactional rollback when watcher creation fails, stop retrying native wait errors, and enforce exact supported process types. Remove the obsolete signal.timeout configuration and update lifecycle listeners accordingly. Document handler concurrency and native shutdown-signal ownership, add deterministic single- and multi-waiter teardown coverage, protect active handler completion, and clean up Signal test typing and stale teardown state.
Record the final Signal architecture trace, accepted findings, deterministic watcher-ownership design, rejected overengineering concerns, regression coverage, performance assessment, API and configuration impact, validation results, and independent review sign-off. Mark Signal complete in the package checklist and advance the audit routing index to Pool with the existing pool-01 and pool-02 revalidation context.
Delete the unused coroutine Context helper and remove the resulting hypervel/context split-package dependency. Declare psr/log directly because the retained KeepaliveConnection extension surface types LoggerInterface. This keeps package metadata aligned with the code and avoids relying on transitive dependencies.
Make heartbeat registration transactional after a KeepaliveConnection acquires and queues its active resource. A timer creation failure now closes that resource, clears connected state, and rethrows the original creation exception. Reuse the same close boundary when a heartbeat callback fails. If a concurrent caller temporarily owns the resource, clear state so its existing finally path drops the resource instead of requeueing it, without replacing the primary heartbeat failure. Cover coroutine creation failure and heartbeat callback failure with regressions that assert disconnected state and exactly one close.
Measure query execution and cumulative-duration thresholds with hrtime instead of wall time. Clock adjustments can no longer produce negative or inflated query durations while the existing number of clock reads remains unchanged. Add a domain regression that supplies a monotonic start time and verifies the elapsed result stays within a realistic millisecond range; the previous wall-clock subtraction fails this check because of the epoch gap.
Measure Redis command success and failure durations with hrtime so wall-clock adjustments cannot produce invalid event timings. The command path retains the same start and end read count and constructs no new hot-path machinery. Extend both CommandExecuted and CommandFailed event regressions to require a present, non-negative elapsed duration while preserving the existing command, parameters, connection, and exception assertions.
Apply the validated pool connect_timeout through each native connection primitive. MySQL and MariaDB receive PDO::ATTR_TIMEOUT, PostgreSQL receives a DSN connect_timeout, and Redis receives its native timeout; explicit driver configuration takes precedence and SQLite remains unchanged. Move pooled idle, release, heartbeat, and generation-lifetime timestamps to one monotonic domain across Pool, Database, Redis, and SimplePool. Existing clock-read counts are preserved, preventing wall-clock changes from prematurely recycling or over-retaining connections without adding a clock abstraction. Correct Frequency to retain exactly its configured ten buckets. Remove its unused Pool reference and the false LowFrequencyInterface constructor requirement, then replace the empty Redis Frequency alias with the Pool implementation directly. Add regression coverage for timeout translation and precedence, lifecycle timestamp domains, exact frequency windows, and callback-created SimplePool connection reuse. The changes add no cancellation wrapper, retry loop, or additional borrow/release work.
Document Pool provenance and the explicit borrow, release, discard, and terminal close ownership model for long-lived workers. Describe min_connections as a lower bound for excess-idle trimming of the total managed count. It does not prewarm, replenish, guarantee idle capacity, or guarantee a total minimum; callers that require a new connection pay its establishment cost. Update the Pool contracts and Database and Redis guides to use the same terminology, explain that lifecycle failures can reduce the managed count below the configured floor, and retain the documented native connect timeout behavior.
Record pool-03 through pool-09 and the linked Database and Redis duration findings, including final ownership boundaries, native timeout precedence, monotonic clock domains, approved interface cleanup, managed-count trimming semantics, and rejected overengineering. Capture the implemented changes, regression coverage, final composer fix results, fresh self-review corrections, independent review sign-off, Laravel-facing result, and hot-path assessment. Mark Pool complete, add the cross-package revalidation routes for Database and Redis, and advance the active audit package to Object Pool.
Document that flushing the factory clears pools shared by every coroutine and should be reserved for boot or test cleanup. Mark recycler interval, timer, start, and stop mutations with their true boot/test boundaries so callers do not reconfigure singleton worker state during requests.
Drop the stale direct nesbot/carbon requirement after confirming the package has no remaining Carbon usage. Retain the Container and Contracts split dependencies because recycler error reporting, coordinator startup, and custom pool-factory resolution still use them.
Isolate each registered pool maintenance transaction so a custom pool that throws cannot starve unrelated pools during the same recycler tick. Report a registry-identity wrapper while retaining the exact original exception, and stop later maintenance only for the failed pool. Remove the unread Container constructor chain from ObjectPool, SimpleObjectPool, PoolManager, and the Sentry transport pool. Update every Object Pool, Sentry, Filesystem, Queue, and Broadcasting construction site without adding a compatibility shim. Add deterministic coverage proving failure reporting, per-pool short-circuiting, and continued maintenance of later pools. Normal borrow, release, proxy, request, job, and transport paths remain unchanged.
Update the completed Object Pool lifecycle design to show the current dependency-minimal PoolManager and SimpleObjectPool constructors. Remove only the obsolete container setup from the snippets while preserving the plan discussion of legitimate container-backed factory resolution.
Record the accepted Object Pool findings, rejected speculative concerns, implementation boundaries, validation evidence, performance assessment, independent review sign-off, and owner approval. Mark Object Pool complete, route the active audit to Process, and preserve the cross-package dependency index without introducing an unsupported revalidation requirement.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (76)
💤 Files with no reviewable changes (13)
📝 WalkthroughWalkthroughThe PR revises signal watcher shutdown, pool timing and timeout handling, object-pool construction and recycler isolation, related package dependencies, documentation, audit plans, and corresponding tests across database, Redis, signal, object-pool, filesystem, queue, and Sentry components. ChangesSignal lifecycle
Pool resource lifecycle
Object-pool lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR hardens resource lifecycle ownership across Signal, Pool, and Object Pool subsystems for long-lived Swoole workers where normal GC and request teardown cannot be relied upon.
Confidence Score: 4/5Safe to merge; no incorrect data paths or broken contracts introduced The changes are cohesive and well-tested. The Frequency window fix is a genuine bug correction. The hrtime migration is consistent across every timing site. The lifecycle hardening in KeepaliveConnection, PoolRecycler, and SignalManager follows correct coroutine-ownership logic. The two P2 observations are a narrow shutdown race in SignalManager and the zero-timeout edge case in configureConnectTimeout — neither causes incorrect behaviour in typical deployments. src/signal/src/SignalManager.php (shutdown race), src/database/src/Pool/DbPool.php (zero-timeout write) Important Files Changed
Reviews (1): Last reviewed commit: "chore(audit): complete object-pool lifec..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Hypervel runs inside long-lived Swoole workers. Resource owners therefore cannot rely on request teardown, periodic polling, or eventual garbage collection to restore a clean state.
This PR is a cohesive lifecycle-hardening batch across Signal, Pool, and Object Pool, including their Database, Redis, Sentry, Filesystem, Queue, and Broadcasting consumers. The common theme is exact ownership: a component either commits a resource completely or cleans it up, and terminal teardown acts on the exact handles it owns.
Signal lifecycle
Signal watchers previously polled
waitSignal()with a five-second timeout. Worker shutdown allows less time by default, so a watcher could still be sleeping when shutdown completed and force Swoole to terminate the process.SignalManagernow waits indefinitely and records only coroutine IDs that are currently parked in the native signal wait. Terminalstop()snapshots and exception-cancels those exact waiters. A handler that has already started is removed from the cancellable set first, so application signal handling is allowed to finish normally.Watcher creation remains transactional: if a later spawn fails, every earlier watcher created by that attempt is canceled. A native
falsewait result is terminal instead of being retried into a busy loop.The old reversible stopped state and
signal.timeoutpolling configuration are removed. The package README now documents nativeSIGTERM/SIGINTownership, process-lifetime handler instances, and the requirement for handlers to be coroutine-safe.Connection pools
Connection lifecycle ownership is tightened in several places:
KeepaliveConnectionthat acquires a resource but fails to create its heartbeat timer now closes the resource and preserves the original creation failure.pool.connect_timeoutis now applied to native connection establishment. MySQL and MariaDB usePDO::ATTR_TIMEOUT, PostgreSQL uses the DSNconnect_timeout, and Redis uses its native top-level timeout. Explicit driver configuration always wins; SQLite remains unchanged.The package also sheds infrastructure that did not own behavior: an unused coroutine Context helper, its package dependency, an unread Pool reference in
Frequency, the false constructor requirement onLowFrequencyInterface, and an empty Redis Frequency alias.psr/logis now a direct dependency because the retained keepalive extension surface types its contract directly.Pool documentation now describes the real ownership model and clarifies that
min_connectionsis a lower bound for excess-idle trimming of the total managed count. It does not prewarm, replenish, guarantee idle capacity, or guarantee a total minimum after failed creation or explicit discard.Object pools
Recycler maintenance is isolated per registered pool. A custom pool that throws is reported with its registry identity and original exception, skips only its own remaining maintenance, and cannot starve later pools during the same tick.
Worker-wide mutators now state their actual lifecycle boundary at both contract and implementation surfaces: factory flushing and recycler timer/start/stop configuration are boot/test operations, not per-request controls.
ObjectPool,SimpleObjectPool,PoolManager, and the Sentry transport pool no longer retain or forward an unread application container. All affected consumers and tests use only the real constructor dependencies. The stale direct Carbon dependency is removed as well.Compatibility and performance
Laravel-facing APIs, configuration, documented behavior, and extension patterns are unchanged.
The changed or removed public surfaces are Hypervel-specific lifecycle APIs: reversible Signal stopped state, the unused Signal polling timeout, the false low-frequency constructor dependency, and unread Object Pool constructor arguments. No compatibility shims are retained on the 0.4 line.
There is no additional request-hot-path lookup, allocation, lock, yield, retry, or clock read:
try/catchper pool during the existing maintenance interval; diagnostic objects are allocated only after a failure.Validation
composer fixpasses at the final branch state:Focused regressions cover native signal-wait teardown, multiple parked watchers, active-handler completion, partial watcher creation failure, keepalive creation and heartbeat failure cleanup, driver timeout translation and precedence, monotonic lifecycle and event timing, frequency window bounds, Object Pool maintenance isolation, constructor updates, and affected consumers.
The Signal, Pool, and Object Pool split manifests validate strictly. Repository-wide stale-reference checks, package-checklist parity, and
git diff --checkare clean.Summary by CodeRabbit
New Features
Bug Fixes
Documentation