Harden Redis pooling, subscriber transport, and topology#453
Conversation
Record the complete Redis pooling, subscriber transport, topology, and Laravel parity design before implementation. Capture the verified phpredis and RESP boundaries, protected Hypervel capabilities, accepted intentional differences, implementation order, regression matrix, hot-path assessment, and anti-overengineering constraints that govern the work unit.
Stop replaying ambiguous Redis failures and classify whether each native generation can be safely retained or must be discarded. Complete current supported phpredis configuration, ACL, context, option, prefix, Sentinel, and Cluster behavior while preserving exact pooled wrapper ownership and callback-immediate release. Add Laravel-shaped macros and boot-only event controls, normalize command routing case-insensitively, reject pool-destructive RESET and sharded subscription operations, remove the incompatible named-proxy extension API, and keep facade and static test cleanup metadata aligned.
Exercise exact wrapper ownership, failure disposition without replay, mixed-case stateful routing, callback-immediate release, macro event granularity, event overrides, and facade metadata. Cover standalone, Sentinel, and Cluster configuration; context and credential normalization; supported native options; prefix precedence; fresh topology resolution; terminal cleanup; and the separate-process RESET safety boundary against live Redis and Valkey behavior.
Replace CRLF-delimited Engine packet framing with one coroutine-hooked PHP stream and an exact RESP2 decoder. Handle complete writes, partial reads, binary bulk payloads, nested arrays, nulls, server errors, malformed lengths, and terminal EOF without corrupting the protocol stream or spinning. Support validated TCP, TLS, IPv4, IPv6, and Unix endpoints with real transport fixtures bound to ephemeral ports and exception-safe teardown.
Retain the first receive-loop failure, close every routing channel exactly once, bound foreground acknowledgements and pings, and prevent cleanup-induced errors from replacing the real cause. Route RESP replies semantically, track channel and pattern subscriptions independently, remove timer-per-message delivery, preserve complete credentials and topology, and make close and worker-shutdown interruption deterministic. Add unit and live-service coverage for binary Pub/Sub, acknowledgement accounting, receive failures, timeouts, channel races, credentials, and teardown.
Always release acquired concurrency leases while keeping the callback failure primary when release also fails. Apply the same direct control flow to both public callback paths without adding a shared abstraction, and distinguish acquisition timeouts from user callbacks that happen to throw the same exception type. Cover the complete callback and release success/failure matrix at both call sites.
Send scaling messages directly through the pooled Redis publisher instead of gating them on the unrelated subscriber connection and retaining an unbounded outage queue. Propagate encoding and Redis failures truthfully, return the actual publish count, and remove the exact pending metric and listener when a metrics request cannot be published while preserving the original failure. Cover disconnected subscribers, Redis outages, response races, publish counts, and cleanup failure precedence.
Copy the configured Hypervel Redis connection instead of reading Laravel's incompatible top-level cluster shape. Hash-tag clustered Horizon prefixes and publish the resolved prefix at both configuration precedence levels so RedisConfig cannot restore a stale source prefix. Verify standalone and Cluster topology, pool settings, credentials, prefix precedence, fallback behavior, and missing connections through the real normalized consumer path.
Format Redis parameters recursively using scalar values and debug types so observation cannot invoke JsonSerializable, string conversion, or other user code after a successful command. Ignore the real pipeline and MULTI opener events strictly and case-insensitively instead of filtering a dead transaction name. Cover nested parameters, objects, closures, resources, throwing serializers, mixed-case batch openers, and boot-time event disabling.
Read Redis span metadata from the normalized database key used by real Hypervel connections instead of the test-only db key. Correct the masking fixture and verify that both successful and failed commands retain a non-zero configured database index.
Document the current phpredis options, connection-local standalone, Sentinel, and Cluster topology, exact subscriber transport, macros, and boot-only event controls. Remove the unsafe automatic replay claim and explain the deliberate pooled RESET, sharded subscription, and connector-extension differences with the supported alternatives. Keep the guide task-focused while preserving Hypervel-specific transforms, SafeScan, raw held access, and native retry and backoff behavior.
Mark Redis complete and record the verified command, subscriber, topology, consumer, metadata, performance, and retained-state findings with their final owning boundaries. Close the carried Redis and pool revalidation items, route the remaining Cache limiter and named-Redis consumer checks to their active or later audits, and make Cache the next work unit. Record the complete validation and review evidence without claiming that pending consumer audits are already complete.
…bscriber-lifecycles
Verify Faker locale selection through exact provider ownership and localized output rather than seeded process-global random draws. Global Mt19937 state is shared across coroutines, so container resolution can yield while another task advances the sequence and makes an exact state-name assertion flaky. Keep the public locale behavior covered without adding RNG isolation, scheduler controls, or any framework source change.
|
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 (1)
📝 WalkthroughWalkthroughRedis pooling, Sentinel and cluster topology, RESP2 subscriber transport, macro/event handling, Reverb publishing, limiter failure precedence, observability, Horizon prefix handling, documentation, and integration tests were updated. Unsupported pooled commands and connector extension APIs were removed, while configuration validation and lifecycle cleanup were expanded. ChangesRedis lifecycle and parity
Estimated code review effort: 5 (Critical) | ~120 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 SummaryThe PR hardens Redis connection ownership, topology handling, subscriber transport, and Laravel-facing compatibility.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/redis/src/PhpRedisConnection.php | Expands standalone and Sentinel connection configuration while preserving verified phpredis context-driven TLS behavior. |
| src/redis/src/PhpRedisClusterConnection.php | Completes Cluster seed, context, option, prefix, and failover handling. |
| src/redis/src/RedisProxy.php | Tightens pooled command ownership and constructs dedicated topology-aware subscribers. |
| src/redis/src/Subscriber/Connection.php | Implements exact RESP2 stream transport with binary-safe reads, complete writes, endpoint normalization, and deterministic failure handling. |
| src/redis/src/Subscriber/Subscriber.php | Coordinates subscription acknowledgements, receive-loop failures, timeouts, and channel-pattern lifecycle state. |
| src/redis/src/RedisConfig.php | Centralizes canonical Redis configuration parsing and topology-specific option normalization. |
| src/redis/src/RedisManager.php | Adds Laravel-shaped macros and boot-time event controls while retaining named pooled connection management. |
| src/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.php | Decouples publishing from subscriber state and removes outage-queue retention. |
| src/horizon/src/Horizon.php | Copies complete named Redis topology and normalizes Cluster-safe Horizon prefixes. |
| src/boost/docs/redis.md | Documents the expanded Redis configuration, topology, subscriber, macro, event, and pooled-operation behavior. |
Reviews (3): Last reviewed commit: "test(redis): cover pool event configurat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
tests/Redis/Subscriber/CommandInvokerTest.php (1)
544-642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNamespace the generic
Connectiontest doubles.
BlockingConnectionandControlledConnectionare collision-prone generic names declared in the sharedHypervel\Tests\Redis\Subscribernamespace. As per coding guidelines, "Use test-specific namespaces for collision-prone generic helper classes, with the test class name as the final namespace segment" — e.g.Hypervel\Tests\Redis\Subscriber\CommandInvokerTest\ControlledConnection.🤖 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/Redis/Subscriber/CommandInvokerTest.php` around lines 544 - 642, Move the test doubles BlockingConnection and ControlledConnection into the test-specific namespace Hypervel\Tests\Redis\Subscriber\CommandInvokerTest, updating any references within CommandInvokerTest accordingly. Keep their implementations unchanged and use the test class name as the final namespace segment.Source: Coding guidelines
tests/Redis/Subscriber/SubscriberTest.php (1)
337-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
readExact()test helper across two subscriber test suites. Both files carry a byte-identical private helper (same docblock, sameRuntimeExceptionmessage); the shared root cause is that no common home exists forRespServer-adjacent read helpers.
tests/Redis/Subscriber/SubscriberTest.php#L337-L357: remove the local helper and use the shared implementation.tests/Redis/Subscriber/CommandInvokerTest.php#L505-L525: remove the local helper and use the shared implementation.Suggested home: a
readExact()method on theRespServerfixture intests/Redis/Fixtures/RespServer.php, or a small trait beside it.🤖 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/Redis/Subscriber/SubscriberTest.php` around lines 337 - 357, The readExact() test helper is duplicated across both subscriber test suites. Add one shared readExact() implementation to the RespServer fixture or an adjacent trait, update tests/Redis/Fixtures/RespServer.php (shared home) accordingly, remove the local helper from tests/Redis/Subscriber/SubscriberTest.php lines 337-357, and remove the local helper from tests/Redis/Subscriber/CommandInvokerTest.php lines 505-525; update both callers to use the shared implementation.src/redis/src/Subscriber/CommandInvoker.php (1)
319-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the 30-second push timeout into a constant.
30.0and the"remained full for 30 seconds"text can drift apart independently.♻️ Proposed refactor
+ private const float MESSAGE_PUSH_TIMEOUT = 30.0; + private function pushMessage(Message $message): void { - if ($this->messageChannel->push($message, 30.0)) { + if ($this->messageChannel->push($message, self::MESSAGE_PUSH_TIMEOUT)) { return; } if (! $this->messageChannel->isTimeout()) { throw new SocketException('The Redis subscriber message channel was closed.'); } $exception = new SocketException(sprintf( - "Redis subscriber message channel [{$message->channel}] remained full for 30 seconds." + 'Redis subscriber message channel [%s] remained full for %s seconds.', + $message->channel, + self::MESSAGE_PUSH_TIMEOUT, ));Note the proposed
private const floattyped constant requires PHP 8.3+.🤖 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/redis/src/Subscriber/CommandInvoker.php` around lines 319 - 343, Update pushMessage to define and reuse a private timeout constant for the 30-second channel push duration, replacing the literal 30.0 and deriving the “remained full for 30 seconds” message from that same constant so the configured duration and text cannot diverge.src/redis/src/RedisConfig.php (1)
88-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that the override only affects pools created afterwards.
RedisPoolsnapshotsconnectionConfig()in its constructor (src/redis/src/Pool/RedisPool.phplines 30-34), so callingenableEvents()/disableEvents()after a pool exists leaves already-created pools/connections on the oldevent.enablevalue. The "boot-only" note captures the intent, but making the pool-snapshot consequence explicit in the docblock would prevent misuse. This is worker-lifetime singleton state under Swoole, so the flag persists for the life of the worker.As per coding guidelines: "Review and report the Swoole persistence impact whenever changing static properties, caches, singleton-held state, manager registries, or other worker-lifetime state."
🤖 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/redis/src/RedisConfig.php` around lines 88 - 113, Update the docblocks for enableEvents() and disableEvents() to state that the override applies only to pools created after the method is called; existing RedisPool instances retain their snapshotted connection configuration for the worker lifetime.Source: Coding guidelines
tests/Redis/PhpRedisClusterConnectionTest.php (1)
229-241: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueValidate the endpoint before starting the server.
If the guard at Line 239 throws,
$server->wait()in thefinallyat Line 259 is never reached and the started server is leaked. Parse and validate the endpoint beforestart().🤖 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/Redis/PhpRedisClusterConnectionTest.php` around lines 229 - 241, Move endpoint parsing and the host/port validation before the `$server->start()` call in the test setup. Keep the existing `InvalidArgumentException` guard and subsequent server startup behavior unchanged, ensuring validation failures occur before the server can be started and leaked.src/redis/src/Limiters/ConcurrencyLimiter.php (1)
105-111: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSuppressed cleanup/release exceptions aren't logged, unlike
closeSubscriber()'s pattern.All three sites deliberately swallow a secondary exception (lease release / listener cleanup) to preserve the primary exception's precedence — the precedence logic itself is correct and well-tested. However, none of them log the swallowed exception, whereas
RedisPubSubProvider::closeSubscriber()(same PR) establishes the pattern of logging suppressed cleanup failures before letting the primary error propagate. Without a log line, an operator debugging a stuck concurrency slot or a leaked metrics listener has no trace that the release/cleanup itself also failed.
src/redis/src/Limiters/ConcurrencyLimiter.php#L105-L111: log the caught$exception(e.g. viaLog::error) before discarding it when$callbackException !== null.src/redis/src/Limiters/ConcurrencyLimiterBuilder.php#L125-L131: apply the same logging when the release exception is suppressed in favor of$callbackException.src/reverb/src/Protocols/Pusher/MetricsHandler.php#L153-L161: log thestopListening()cleanup failure in the innercatch (Throwable)block instead of only a comment.🤖 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/redis/src/Limiters/ConcurrencyLimiter.php` around lines 105 - 111, Log suppressed cleanup exceptions before discarding them, while preserving primary exception precedence: in src/redis/src/Limiters/ConcurrencyLimiter.php lines 105-111 and src/redis/src/Limiters/ConcurrencyLimiterBuilder.php lines 125-131, log the caught release exception when $callbackException is present; in src/reverb/src/Protocols/Pusher/MetricsHandler.php lines 153-161, log the inner stopListening() failure instead of only commenting on it.src/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.php (1)
42-42: 🩺 Stability & Availability | 🔵 TrivialRemoving
queuedPublishesimproves worker-lifetime memory safety.
publish()now always goes straight through the pooledRedisProxy, independent of subscriber connection state, so no buffering is needed. This also removes a singleton-held, unbounded-growth-prone buffer (queuedPublishes) that could accumulate messages for the lifetime of the worker during prolonged disconnects — a net improvement worth calling out given this touches worker-lifetime state.As per coding guidelines: "Review and report the Swoole persistence impact whenever changing static properties, caches, singleton-held state, manager registries, or other worker-lifetime state."
Also applies to: 175-178, 208-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.php` at line 42, Remove the singleton-held queuedPublishes buffer and all related queueing/flush logic from RedisPubSubProvider, including its declaration and references near publish(). Ensure publish() always sends directly through the pooled RedisProxy regardless of subscriber connection state, and document the Swoole worker-lifetime memory-safety impact of removing this unbounded state.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 `@docs/plans/2026-07-25-1811-redis-pooling-subscriber-and-laravel-parity.md`:
- Around line 1573-1584: Update the Redis dependency changes described under
“Split metadata” to also add hypervel/macroable to the root composer.json using
composer require, while retaining the direct dependency in
src/redis/composer.json. Verify both manifests remain aligned and pass metadata
validation.
- Line 1253: Update the Horizon prefix assignment using an explicit
null/empty-value check instead of the truthiness-based `?:` operator, so a
configured prefix of `"0"` is preserved while null or empty configuration still
follows the intended default policy.
- Around line 1161-1191: Clarify and test the worker-lifetime behavior of
RedisConfig::enableEvents() and disableEvents(): after either call, every
subsequently assembled connection config, including configs for existing and
later pools managed by RedisManager, must reflect the override through
connectionConfig(). Document this shared-container call-boundary behavior in the
method docblocks and add coverage for both enable and disable paths without
adding a reset API.
- Around line 513-522: Update Subscriber\Connection’s stream-context
normalization to preserve an existing $context['stream'] shape without wrapping
it again under 'ssl'. Branch between already-normalized stream contexts and raw
flat or $context['ssl'] options, adding the SSL wrapper only for the latter;
cover both forms at the subscriber stream boundary.
In `@src/redis/README.md`:
- Line 8: Update the “Differences From Laravel” heading in the Redis README to
use the configured setext heading style required by MD003, matching the
surrounding documentation conventions.
In `@src/redis/src/RedisProxy.php`:
- Around line 578-585: Update RedisProxy::subscriber() at the createSubscriber
call to pass the cluster’s configured scheme instead of null, using the scheme
from the cluster configuration (including tls:// seeds or scheme => 'tls') while
leaving the pooled connection scheme unchanged.
In `@src/testing/src/PHPUnit/AfterEachTestSubscriber.php`:
- Line 246: Update src/testing/composer.json to declare hypervel/redis as a
required dependency, matching the RedisConnection::flushMacros() reference in
AfterEachTestSubscriber. Use the project’s existing dependency version
conventions and ensure consumers installing hypervel/testing receive the Redis
package automatically.
In `@tests/Redis/RedisConnectionTest.php`:
- Around line 834-900: Ensure every macro registered by the RedisConnection
macro tests is removed after each test, rather than relying on test ordering.
Add teardown cleanup or register an AfterEachTestSubscriber hook to call
RedisConnection::flushMacros(), covering registrations from
testMixinRegistersConnectionMacros,
testMacroLookupPreservesExactNamesAndMayShadowNativeCommands, and
testRedisExceptionInsideMacroUsesConnectionDispositionRule.
In `@tests/Redis/RedisProxyTest.php`:
- Around line 65-86: Move macro cleanup out of
testMacroRegistrationMethodsDoNotCheckoutRedis and into tearDown or the test
suite’s centralized after-each cleanup so it runs even when assertions fail.
Ensure RedisProxy/RedisConnection static macros such as greeting and farewell
are flushed after every test, and remove the inline flushMacros call from the
test.
---
Nitpick comments:
In `@src/redis/src/Limiters/ConcurrencyLimiter.php`:
- Around line 105-111: Log suppressed cleanup exceptions before discarding them,
while preserving primary exception precedence: in
src/redis/src/Limiters/ConcurrencyLimiter.php lines 105-111 and
src/redis/src/Limiters/ConcurrencyLimiterBuilder.php lines 125-131, log the
caught release exception when $callbackException is present; in
src/reverb/src/Protocols/Pusher/MetricsHandler.php lines 153-161, log the inner
stopListening() failure instead of only commenting on it.
In `@src/redis/src/RedisConfig.php`:
- Around line 88-113: Update the docblocks for enableEvents() and
disableEvents() to state that the override applies only to pools created after
the method is called; existing RedisPool instances retain their snapshotted
connection configuration for the worker lifetime.
In `@src/redis/src/Subscriber/CommandInvoker.php`:
- Around line 319-343: Update pushMessage to define and reuse a private timeout
constant for the 30-second channel push duration, replacing the literal 30.0 and
deriving the “remained full for 30 seconds” message from that same constant so
the configured duration and text cannot diverge.
In `@src/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.php`:
- Line 42: Remove the singleton-held queuedPublishes buffer and all related
queueing/flush logic from RedisPubSubProvider, including its declaration and
references near publish(). Ensure publish() always sends directly through the
pooled RedisProxy regardless of subscriber connection state, and document the
Swoole worker-lifetime memory-safety impact of removing this unbounded state.
In `@tests/Redis/PhpRedisClusterConnectionTest.php`:
- Around line 229-241: Move endpoint parsing and the host/port validation before
the `$server->start()` call in the test setup. Keep the existing
`InvalidArgumentException` guard and subsequent server startup behavior
unchanged, ensuring validation failures occur before the server can be started
and leaked.
In `@tests/Redis/Subscriber/CommandInvokerTest.php`:
- Around line 544-642: Move the test doubles BlockingConnection and
ControlledConnection into the test-specific namespace
Hypervel\Tests\Redis\Subscriber\CommandInvokerTest, updating any references
within CommandInvokerTest accordingly. Keep their implementations unchanged and
use the test class name as the final namespace segment.
In `@tests/Redis/Subscriber/SubscriberTest.php`:
- Around line 337-357: The readExact() test helper is duplicated across both
subscriber test suites. Add one shared readExact() implementation to the
RespServer fixture or an adjacent trait, update
tests/Redis/Fixtures/RespServer.php (shared home) accordingly, remove the local
helper from tests/Redis/Subscriber/SubscriberTest.php lines 337-357, and remove
the local helper from tests/Redis/Subscriber/CommandInvokerTest.php lines
505-525; update both callers to use the shared implementation.
🪄 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: e3968944-5414-43e1-83ca-b30e9597c6bf
📒 Files selected for processing (60)
docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-07-25-1811-redis-pooling-subscriber-and-laravel-parity.mdsrc/boost/docs/redis.mdsrc/horizon/src/Horizon.phpsrc/redis/README.mdsrc/redis/composer.jsonsrc/redis/src/Limiters/ConcurrencyLimiter.phpsrc/redis/src/Limiters/ConcurrencyLimiterBuilder.phpsrc/redis/src/PhpRedisClusterConnection.phpsrc/redis/src/PhpRedisConnection.phpsrc/redis/src/RedisConfig.phpsrc/redis/src/RedisConnection.phpsrc/redis/src/RedisManager.phpsrc/redis/src/RedisProxy.phpsrc/redis/src/RedisSentinelFactory.phpsrc/redis/src/RedisServiceProvider.phpsrc/redis/src/Subscriber/CommandInvoker.phpsrc/redis/src/Subscriber/Connection.phpsrc/redis/src/Subscriber/Constants.phpsrc/redis/src/Subscriber/Exceptions/ServerException.phpsrc/redis/src/Subscriber/Subscriber.phpsrc/reverb/src/Protocols/Pusher/MetricsHandler.phpsrc/reverb/src/Servers/Hypervel/Scaling/RedisPubSubProvider.phpsrc/sentry/src/Features/RedisFeature.phpsrc/support/src/Facades/Redis.phpsrc/telescope/src/Watchers/RedisWatcher.phpsrc/testing/src/PHPUnit/AfterEachTestSubscriber.phptests/Integration/Foundation/FoundationHelpersTest.phptests/Integration/Horizon/Feature/RedisPrefixTest.phptests/Integration/Redis/RedisConnectionIntegrationTest.phptests/Integration/Redis/RedisConnectorTest.phptests/Integration/Redis/RedisResetIntegrationTest.phptests/Integration/Redis/Subscriber/SubscriberIntegrationTest.phptests/Redis/ConcurrencyLimiterBuilderTest.phptests/Redis/ConcurrencyLimiterTest.phptests/Redis/Fixtures/PhpRedisClusterConnectionStub.phptests/Redis/Fixtures/PhpRedisConnectionStub.phptests/Redis/Fixtures/RespServer.phptests/Redis/Fixtures/Tls/server.crttests/Redis/Fixtures/Tls/server.keytests/Redis/MultiExecTest.phptests/Redis/PackageMetadataTest.phptests/Redis/PhpRedisClusterConnectionTest.phptests/Redis/RedisConfigTest.phptests/Redis/RedisConnectionTest.phptests/Redis/RedisEventsTest.phptests/Redis/RedisManagerTest.phptests/Redis/RedisPoolHeartbeatTest.phptests/Redis/RedisProxyNonCoroutineTest.phptests/Redis/RedisProxyTest.phptests/Redis/RedisSentinelFactoryTest.phptests/Redis/Subscriber/CommandInvokerTest.phptests/Redis/Subscriber/ConnectionTest.phptests/Redis/Subscriber/SubscriberTest.phptests/Reverb/Protocols/Pusher/MetricsHandlerTest.phptests/Reverb/Servers/Hypervel/Scaling/RedisPubSubProviderTest.phptests/Sentry/Features/RedisIntegrationTest.phptests/Telescope/Watchers/DisabledWatcherTest.phptests/Telescope/Watchers/RedisWatcherTest.php
💤 Files with no reviewable changes (2)
- src/redis/src/Subscriber/Constants.php
- tests/Redis/Fixtures/PhpRedisClusterConnectionStub.php
Make Horizon's shipped configuration the sole owner of its application-scoped Redis prefix default. The runtime consumer now uses the typed config getter without a second fallback, while regression coverage proves both missing and blank environment values and verifies Cluster-safe publication at every effective prefix location. Consolidate exact RESP test reads in the shared server fixture, resolve Cluster test endpoints before starting handler coroutines, and derive the subscriber message-channel timeout and diagnostics from one typed constant. Update the Redis implementation plan and lifecycle ledger to record the final boundaries.
|
@greptile-apps @coderabbitai I went through every inline comment, main-thread finding, and nitpick against the current source, the phpredis implementation, and the regression suite. Follow-up commit
The transport findings do not require changes. phpredis itself selects SSL for a bare host with a non-empty context; this was verified in its C source and with a real TLS ClientHello. The subscriber context conversion accepts the three documented phpredis shapes and wraps the selected SSL options exactly once for PHP streams. Cluster-discovered nodes inherit The event override already documents and tests its boot-only, future-pool boundary. Existing pools intentionally keep their snapshotted configuration. The root and Testing dependency suggestions do not match the monorepo or framework cleanup dependency model. Redis macro cleanup is already failure-safe in the centralized after-each subscriber; per-class teardown would duplicate that owner. The remaining heading, helper-namespace, suppressed-cleanup logging, blanket docstring, and source-comment suggestions either conflict with repository conventions or add ceremony without a supported failure. Reverb queue removal and its worker-memory benefit are already recorded in the plan and ledger. The authoritative |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- follow-up commit ---'
git show --stat --oneline ac665c5f6
git show --format=fuller --name-status --find-renames ac665c5f6 -- \
src/horizon \
tests/Redis/Fixtures/RespServer.php \
tests/Redis \
src/redis \
| sed -n '1,260p'
echo '--- candidate files ---'
fd -a -t f 'Horizon.php|RespServer.php|RedisProxyTest.php|CommandInvoker.php|Constants.php|RedisConfig.php|Connection.php|PhpRedisClusterConnection.php' . \
| sort
echo '--- structural outlines ---'
for file in \
src/horizon/src/Horizon.php \
tests/Redis/Fixtures/RespServer.php \
src/redis/src/Subscriber/CommandInvoker.php \
src/redis/src/RedisConfig.php \
src/redis/src/RedisProxy.php \
src/redis/src/Subscriber/Connection.php \
src/redis/src/PhpRedisClusterConnection.php
do
if [ -f "$file" ]; then
echo "### $file"
ast-grep outline "$file" --items all 2>/dev/null | sed -n '1,220p'
fi
done
echo '--- relevant symbols and assertions ---'
rg -n -C 5 -P \
'Horizon::use|function use\s*\(|prefix|readExact|MESSAGE_CHANNEL|messageChannel|endpoint\(|createSubscriber|cluster\.context|eventsOverride|enableEvents|disableEvents|normalizeContext' \
src/horizon/src/Horizon.php \
tests/Integration/Horizon/Feature/RedisPrefixTest.php \
tests/Redis/Fixtures/RespServer.php \
tests/Redis/RedisProxyTest.php \
tests/Redis/Subscriber/CommandInvokerTest.php \
src/redis/src/Subscriber/CommandInvoker.php \
src/redis/src/Subscriber/Constants.php \
src/redis/src/RedisProxy.php \
src/redis/src/RedisConfig.php \
src/redis/src/Subscriber/Connection.php \
src/redis/src/PhpRedisClusterConnection.php \
2>/dev/null | sed -n '1,900p'Length of output: 55449 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- exact follow-up diff ---'
git diff ac665c5f6^ ac665c5f6 -- \
src/horizon/config/horizon.php \
src/horizon/src/Horizon.php \
src/redis/src/Subscriber/CommandInvoker.php \
tests/Horizon/HorizonConfigTest.php \
tests/Integration/Horizon/Feature/RedisPrefixTest.php \
tests/Redis/Fixtures/RespServer.php \
tests/Redis/PhpRedisClusterConnectionTest.php \
tests/Redis/RedisProxyTest.php \
tests/Redis/Subscriber/CommandInvokerTest.php \
tests/Redis/Subscriber/SubscriberTest.php \
| sed -n '1,1000p'
echo '--- shipped Horizon configuration and config test ---'
sed -n '1,180p' src/horizon/config/horizon.php
sed -n '1,240p' tests/Horizon/HorizonConfigTest.php
echo '--- timeout diagnostic implementation ---'
sed -n '315,355p' src/redis/src/Subscriber/CommandInvoker.php
echo '--- all exact-read helper definitions and fixture use sites in Redis tests ---'
rg -n -P 'function\s+readExact\s*\(|::readExact\s*\(' tests/Redis -g '*.php'
echo '--- endpoint ordering in follow-up test contexts ---'
sed -n '1,260p' tests/Redis/PhpRedisClusterConnectionTest.php
sed -n '1060,1140p' tests/Redis/RedisProxyTest.php
echo '--- current authored CodeRabbit review comments on this PR ---'
gh api repos/hypervel/components/pulls/453/comments \
--paginate \
--jq '.[] | select(.user.login == "coderabbitai") | {id, path, line, body}' \
| sed -n '1,1200p'Length of output: 37457
The follow-up correctly:
I also confirmed the documented event override boundary, and that Cluster-discovered subscriber endpoints receive 🐇 ✅ ✏️ Learnings added
|
|
The follow-up commit addresses everything cleanly. Summary of what landed: Horizon prefix ownership —
Cluster test endpoint resolution — moved before the handler coroutine starts, as flagged.
The rationale for the unchanged transport findings holds up: phpredis selecting SSL from a non-empty context (not scheme) is documented behavior confirmed in the C source, the three-shape context conversion matches the documented phpredis API surface, and Cluster nodes inheriting Everything that was actionable is resolved. The PR is clear to merge. |
Exercise the boot-only Redis event override against a live RedisPool so the documented non-retrofit boundary is enforced directly. Build the pool from a real configuration repository, change the worker-wide override, and prove the existing pool retains its captured setting while subsequent configuration assembly receives the new value.
1 participant
Summary
This completes the Redis lifecycle audit and fixes the parts of the package that were still built around unsafe retry, incomplete topology, or line-oriented subscriber assumptions.
The main change is ownership clarity. A Redis command is executed once. The checked-out connection is then either retained or discarded based on its actual protocol state; the framework no longer reconnects and replays a command whose server-side outcome may be unknown. Stateful operations continue to pin the exact wrapper to the current coroutine, and callback-form pipelines and transactions still return that connection as soon as the callback finishes.
Subscriber traffic now uses a dedicated coroutine-hooked stream with exact RESP2 decoding. It handles fragmented reads, complete writes, binary payloads, protocol errors, TLS, Unix sockets, IPv6, Sentinel master discovery, and Cluster master selection without CRLF packet framing or a timer coroutine per message.
The change also brings the supported phpredis surface up to current Laravel conventions where they fit Hypervel's pooled architecture, while preserving Hypervel's transforms, SafeScan helpers, raw held access, connection-local topology, and immediate callback release.
For more detailes, see: docs/plans/2026-07-25-1811-redis-pooling-subscriber-and-laravel-parity.md
Redis command and pool behavior
RedisException.READONLYandMASTERDOWNwithout reconnecting standalone endpoints that cannot change.RESETbefore native dispatch because it can terminate PHP in pipeline mode and otherwise clears authentication and database state owned by the pool.Connection configuration and Laravel-facing APIs
enableEvents()anddisableEvents()controls that affect future pools without reading configuration on every command.extend()API, which had Laravel's method name but incompatible semantics. Macros and held raw access remain the supported extension points.Subscriber transport and lifecycle
Linked packages
Documentation and audit records
Test reliability
The full merged gate exposed an existing Faker locale test that asserted one exact draw from process-global
mt_rand(). Container resolution may yield in Hypervel's coroutine runtime, allowing another coroutine to advance that shared sequence. The test now verifies exact locale-provider ownership and localized output instead of depending on global RNG ordering. It does not depend on the host locale, country, or timezone.Validation
composer fixgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
reset, pooledssubscribe).Documentation
Tests