Skip to content

[6.x] Let sites check in from CLI, scheduler, and front-end requests. - #15208

Open
jackmcdade wants to merge 3 commits into
6.xfrom
feature/outpost-heartbeat
Open

[6.x] Let sites check in from CLI, scheduler, and front-end requests.#15208
jackmcdade wants to merge 3 commits into
6.xfrom
feature/outpost-heartbeat

Conversation

@jackmcdade

Copy link
Copy Markdown
Member

Outpost currently only hears from the Control Panel. Checking in after responses, on an hourly schedule, and after artisan/please runs gives us a heartbeat for sites that never open the CP — the foundation for us to provide significantly better site tooling.

Outpost currently only hears from the Control Panel. Checking in after responses, on an hourly schedule, and after artisan/please runs gives us a heartbeat for sites that never open the CP — the foundation for better site tooling.

Co-authored-by: Cursor <cursoragent@cursor.com>

@jasonvarga jasonvarga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice — the Radio seam is the right shape, and the ignore list correctly keeps schedule:* out so the hourly task can't double-fire through the CommandFinished listener. One thing needs fixing before this can go in.

Console pings defeat the Outpost response cache

Outpost::hasCachedResponse() only accepts the cached response when the current payload matches the cached one, excluding just two keys:

private function payloadHasChanged($previous, $current)
{
    $exclude = ['ip', 'php_version'];

    return Arr::except($previous, $exclude) !== Arr::except($current, $exclude);
}

port isn't excluded, and payload() sets it from request()->server('SERVER_PORT'). The CLI SAPI never populates that:

$ php -r 'var_dump(isset($_SERVER["SERVER_PORT"]));'
bool(false)

So port is null in console and 80/443 on the web.

That didn't matter before this PR, because Outpost::radio() was only reachable from web contexts — the CP middleware and PhoneHomeController. (ReportThemeUsage is the only console-adjacent consumer and it posts to /v3/theme, never calling radio().) This PR adds two console callers, so the payloads now alternate and every console↔web flip is a cache miss:

  1. Hourly cron fires → port => null → miss → HTTP request → caches payload with port => null
  2. Next front-end request → port => 443 → miss → HTTP request inside terminate(), 5s Guzzle timeout, holding a PHP-FPM/Octane worker and statamic.outpost.lock for the duration
  3. Next non-ignored command (migrate, statamic:*, about, anything custom) → port => null → miss again

Instead of roughly one round-trip an hour, a site running the scheduler with normal traffic hits Outpost on essentially every context switch — and the front-end half of that is synchronous in terminate().

resolveHost() shows the problem was anticipated for host; port just needs the same treatment. Worth noting resolveHost() is only a partial fix for host too — it falls back to parse_url(config('app.url')), which still won't match request()->getHost() on the very common APP_URL=http://localhost, on example.com vs www.example.com, or on any multisite install serving several domains.

Simplest fix is to treat both as environment noise, the same as ip:

$exclude = ['ip', 'port', 'host', 'php_version'];

That covers the console case and the APP_URL/multisite mismatch in one go, and resolveHost() could then go away.


Separately, and worth a look while you're in here: Outpost::request() takes the lock before checking the cache, and Outpost isn't a singleton, so every front-end request now does a distributed lock acquire/release plus a full payload() build (Addon::all() included) — all front-end traffic serialising through one lock key. When Outpost is slow the lock holder burns the full 5s while concurrent requests block on $lock->block(5), then hit LockTimeoutExceptioncacheAndReturnErrorResponse(), which caches ['error' => …] for 5 minutes. That's the same cached response LicenseManager reads for the CP's licensing state. A short-TTL "recently pinged" marker in Radio, checked before touching Outpost, would keep the vast majority of requests out of that path entirely.

…ached response, and throttle Radio pings with a short-lived marker.

Co-authored-by: Cursor <cursoragent@cursor.com>
@jackmcdade

Copy link
Copy Markdown
Member Author

Addressed in cc1f6cb:

  • payloadHasChanged() now excludes host, ip, port, and php_version, so console↔web flips (and APP_URL/multisite host mismatches) reuse the cached response. resolveHost() is gone and the payload is back to plain request()->getHost().
  • Took your suggestion on the lock contention too: Radio::ping() now checks a statamic.outpost.pinged marker (5 min TTL, same store as the lock/response cache) before touching Outpost. It's set before the radio call, so concurrent requests bail immediately instead of queuing on the lock while one request does the round-trip. It's also set on failure, matching Outpost's error-cache behavior.

Added tests for the cached-response reuse and the throttle, plus an outpost store flush in PhoneHomeTest/RadioTest setup since the file-based test store persisted the marker across runs.

@jasonvarga jasonvarga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fixes look right. First, though, a correction I owe you.

I had the mechanism wrong

I claimed payload()['port'] is null in console because the CLI SAPI doesn't populate $_SERVER['SERVER_PORT']. The php -r check I ran was accurate about the raw CLI environment but irrelevant, because Laravel never uses that environment for request() in console.

Illuminate\Foundation\Bootstrap\SetRequestForConsole sits in the console kernel's bootstrappers and synthesizes the request from config('app.url'):

$app->instance('request', Request::create($uri, 'GET', [], [], [], $server));

Request::create() derives SERVER_PORT from the scheme:

https://example.com          host='example.com'  port=443
http://localhost             host='localhost'    port=80
https://example.com:8443     host='example.com'  port=8443

So console payloads carry a real host and port. The divergence I described is conditional, not universal — it happens when APP_URL doesn't reflect how the site is actually served (the untouched http://localhost default, example.com vs www., http/https behind an untrusted proxy) and on any multisite install serving several domains from one APP_URL. On those the thrash and the blocking terminate() call happen as described; on a correctly configured single-domain site they don't. That's a Warning, not the Critical I filed it as. Sorry for the overstatement.

The fix you've landed is still the right one, and it incidentally confirms resolveHost() was safe to delete — it re-derived the host from app.url, which is exactly what SetRequestForConsole had already done. Good riddance to it.

On the fixes

The exclude list and the throttle both do the job, and the_cached_response_is_used_when_only_the_environment_has_changed and it_throttles_pings pin the behaviour rather than restating it. Setting the marker before the Outpost call so a failure throttles too is the right instinct — that's the case that would otherwise hammer a down Outpost on every request.

Non-blocking things I'd still look at:

The throttle is a non-atomic check-then-set.

if ($this->recentlyPinged()) {
    return;
}

$this->markAsPinged();

has() then put() isn't atomic, so at marker expiry a burst of concurrent requests all see no marker, all pass, and all pile into Outpost::radio() and contend on statamic.outpost.lock — the exact herd the throttle is there to prevent. Outpost's own lock keeps it correct, but the throttle misses the one moment it matters. Cache::add() is the atomic put-if-absent and returns false when the key exists, which collapses both helpers into the guard:

if (! $this->cache()->add(self::PING_CACHE_KEY, now()->timestamp, self::PING_INTERVAL)) {
    return;
}

Excluding host makes domain changes eventually-consistent. Licences are domain-bound, and host used to be part of the change signal — moving a site to a new domain forced an immediate re-validation, where now it waits out the hour TTL. Almost certainly the right trade, and it's what makes multisite stop thrashing, so I'd just flag it as a deliberate choice rather than something to change.

PhoneHomeController is now throttled. It routes through Radio::ping(), so it's a silent no-op within 5 minutes of any other ping — including the front-end ping any traffic will have just caused. If that endpoint is meant to be a "check in right now" lever, it probably wants to bypass the throttle.

Minor: RadioTest and PhoneHomeTest both needed a Cache::store('outpost')->flush() in setUp() for the marker. Downstream site and addon suites hitting CP routes will hit the same order-dependence without knowing to do that.


Unrelated to the code: P8.4 - L12.* - prefer-stable - ubuntu-latest - shard 1/4 has been hung ~15 minutes on the "Update apt sources" step, before dependencies or tests run. Every sibling shard passed. Wants a re-run.

… it, and skip during-request pings in unit tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
@jackmcdade

Copy link
Copy Markdown
Member Author
ping-pong-playing-robots

@jackmcdade

Copy link
Copy Markdown
Member Author

All four taken in 77f028b:

  • Atomic throttle: ping() now guards with Cache::add() exactly as you sketched — no more check-then-set window at marker expiry.
  • Phone-home: added Radio::forcePing(), which skips the guard but still refreshes the marker (so a manual check-in throttles the ambient pings that follow). The controller uses it, keeping that endpoint a "check in right now" lever.
  • Downstream order-dependence: shouldPingDuringRequest() now returns false under unit tests, matching shouldPingAfterResponse(). Downstream site/addon suites hitting CP routes never touch the marker (or Outpost) at all, so no flush knowledge required. That let me drop the PhoneHomeTest flush too, since forcePing() doesn't read the marker.
  • host exclusion: agreed it's a deliberate trade — domain moves now wait out the hour TTL instead of forcing an immediate re-validation, in exchange for multisite and APP_URL mismatches not thrashing. Comfortable with that.

Thanks for the SetRequestForConsole correction — good to know the thrash is conditional on APP_URL accuracy rather than universal. Doesn't change the fix, but it's a better mental model.

The hung shard was stuck pre-dependencies, so I cancelled that run; the new commit kicked off a fresh one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants