Working toward Octane support: worker-safe primitives - #241
Open
austinderrick wants to merge 6 commits into
Open
Working toward Octane support: worker-safe primitives#241austinderrick wants to merge 6 commits into
austinderrick wants to merge 6 commits into
Conversation
…t application server Under PHP-FPM every request gets a fresh process, so state left on a long-lived object never hurts anyone. Under Laravel Octane the same process serves many requests, and anything derived from one request stays visible to the next. This adds the primitives the companion Winter changes call. Fires listeners registered against the interfaces an event carries, not only against its class name. Laravel dispatches several events by interface and Octane depends on it: RequestTerminated carries OperationTerminated, and Octane registers its cleanup against the interface. With the previous dispatcher, Octane's own state handling sat inactive for the whole request lifecycle and nothing errored. This applies everywhere rather than only under Octane, and is the widest reaching change here. Adds Application::runningInApplicationServer(), which reports whether a request-serving worker is active by testing for the Laravel\Octane\Contracts\Client binding. Octane binds it in workers and nowhere else, so installing the package does not make an ordinary artisan command answer true. Boot-time gates need this because runningInConsole() is true under Octane, where the SAPI really is CLI. Adds reset primitives to the services that cache per-request values, named by the new Winter\Storm\Contracts\ResetsWorkerState contract so Winter can find them. Auth\Manager held the resolved user and impersonation stack, Halcyon\MemoryCacheManager and Halcyon\Model held parsed template records, Exception\ErrorHandler held custom handler masks, and ExtendableTrait held per-request extension bindings. Implementations must be idempotent, and must leave boot-time registrations alone, since those are built once per worker and discarding them degrades the worker for good. Applies the console root URL override in RegisterWinter only when no request is bound. Under Octane the SAPI is CLI, so it previously fired for every request a worker served and overwrote the URL derived from the real request. Documents how flushRequestCache differs from flushDuplicateCache, which are easy to confuse. Tests: 736 green, each verified failing before its fix. Also exercised end to end against a Winter based application of about thirty plugins running on Octane and FrankenPHP, whose Cypress suite of 36 groups passes on Octane and PHP-FPM alike.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
austinderrick
marked this pull request as draft
August 12, 2026 00:53
austinderrick
marked this pull request as ready for review
August 12, 2026 01:07
The worker-state reset ran before the trusted-proxy middleware, so an eager Request::ip() there recorded the load balancer's address — and could still be reading the previous operation's request. resetWorkerState() now leaves the address unresolved and resolveIpAddress() derives it on first use, which under throttling happens mid-request, after the proxy chain is trusted. Also corrects the runningInApplicationServer() docblock: Octane's worker entrypoints set APP_RUNNING_IN_CONSOLE=false, so runningInConsole() answers false inside a worker, not true as previously claimed.
Coalescing a null Request::ip() to '0.0.0.0' changed throttle semantics for queue jobs and artisan commands: findThrottleByUserId() scopes its lookup to the given address when one is present, so the placeholder address missed existing ban rows recorded under real client IPs and minted spurious '0.0.0.0' rows. A null address means 'match on the user alone', which is how console contexts found bans before this branch. The resolver is now public (getIpAddress) so external readers have a resolving access path instead of observing the property's unresolved null window.
init() still captured Request::ip() at singleton construction, which under plain PHP-FPM can happen before the trusted-proxy middleware runs — anything resolving auth early froze the load balancer's address for the whole request, and getIpAddress() never re-derives once a non-null value is stored. The address now starts unresolved everywhere and is derived at first actual use.
austinderrick
force-pushed
the
feat/octane-worker-support
branch
from
August 13, 2026 23:50
3ad24fb to
18d39fe
Compare
The worker-state and detection tests carried multi-paragraph docblocks narrating production rationale that belongs in the production code, plus one claim the branch itself later corrected (that a worker answers true to runningInConsole). Cut each comment to what the test name does not already say, and correct that claim where it was repeated.
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.
Summary
Working toward Octane support. Required by wintercms/winter#1511.
The Octane integration itself lives in a first-party plugin, Winter.Octane. The plugin registers Octane's service provider, clears request state at the start of every operation, and carries the worker test suite. This PR contains the Storm changes any integration of that kind needs: services that can discard request state, a way to detect a worker, and an event dispatcher that fires the listeners Octane relies on.
Under PHP-FPM every request gets a fresh process, so state left on a long-lived object never hurts anyone. Under Laravel Octane the same process serves many requests. Anything derived from one request stays visible to the next, and some of it belongs to a different user. Each change below fixes a place where Storm assumes the process is about to die.
Two of the changes also affect PHP-FPM behavior and deserve a closer look: the dispatcher fix and the throttle address fix. Both are called out below. Everything else only runs when an application server calls it.
Event dispatch
Winter\Storm\Events\Dispatcheronly fired listeners registered against an event's own class name, never against the interfaces the event implements. Laravel dispatches several of its events by interface, and Octane relies on that:RequestTerminatedcarries theOperationTerminatedinterface, and Octane registers its cleanup against the interface rather than the class. With Winter's dispatcher in place, that cleanup never ran, and nothing errored to show it. The dispatcher now also fires listeners bound to the interfaces an event carries.tests/Events/InterfaceListenerTest.phpcovers this.Application server detection
Several boot-time gates in Winter ask whether the process is serving the back end or running in the console, and neither question identifies a worker. Octane's worker entrypoints set
APP_RUNNING_IN_CONSOLEto false before the application is created, sorunningInConsole()answers false inside a worker, but that only separates a worker from an artisan command, not from an ordinary PHP-FPM request.Application::runningInApplicationServer()reports whether a request-serving worker is active. It tests whetherLaravel\Octane\Contracts\Clientis bound, which Octane binds in workers and nowhere else. Installing the package does not bind it, so an ordinary artisan command still answers false.tests/Foundation/ApplicationServerDetectionTest.phpcovers this.Worker state reset
A group of Storm services cache values for the life of one request. Each one now exposes a way to discard that state, and
Winter\Storm\Contracts\ResetsWorkerStatenames the contract so Winter can find them.The services and what they were holding:
Auth\Managerkept the resolved user and the impersonation stack. Left in place, one visitor's session could answer for the next.Halcyon\MemoryCacheManagerandHalcyon\Modelkept parsed template records that outlived the request that loaded them.Exception\ErrorHandlerkept custom handler masks, so a handler registered by one request stayed registered for later ones.Extension\ExtendableTraitkept per-request extension bindings.Auth\Manageralso no longer captures the client IP address when it is constructed. Construction, and under a worker the operation boundary, can both happen before the trusted-proxy middleware has run, and an address captured that early is the load balancer's rather than the client's.getIpAddress()now derives the address the first time something needs it, which under throttling happens mid-request, after the proxy chain is trusted. In console contextsRequest::ip()is null, and the null is kept rather than replaced with a placeholder, because throttle lookups treat a null address as "match on the user alone" and a placeholder would hide existing ban records. This is the second change that applies under PHP-FPM as well.The contract asks two things of an implementation. It must be safe to run more than once for a single operation, including after one that threw partway through. And it must leave boot-time registrations alone: aliases and event listeners are built once per worker, and throwing them away leaves the worker degraded for good. That second rule caused me more trouble than the first.
tests/Foundation/WorkerStateTest.phpcovers this.Console bootstrap
RegisterWinterforced the root URL from config whenever the application reported a console context. A worker boots from a console context, so the override could apply to worker-served traffic and overwrite the URL derived from the real request, pointing generated links at the configured host rather than the one the visitor used. The bootstrapper now applies the override only when no request is bound, which is the case it was written for.tests/Foundation/Bootstrap/RegisterWinterTest.phpcovers this.Documentation
flushRequestCacheandflushDuplicateCachesound alike and do different jobs. The docblocks now say which is which, since picking the wrong one during this work was easy to do.Testing
New test files:
tests/Events/InterfaceListenerTest.phptests/Foundation/ApplicationServerDetectionTest.phptests/Foundation/WorkerStateTest.phptests/Foundation/Bootstrap/RegisterWinterTest.php