CI ran `yarn && yarn dev` with no committed yarn.lock, so every run resolved
the latest matching versions and dependency drift broke the pipeline in two
independent ways:
- select2 4.1.0 added engines.node ">=24" but the runner used node 22, so
`yarn install` failed outright on every pull request.
- webpack 5.108 removed lib/SizeFormatHelpers, which laravel-mix 6 still
requires, so `yarn dev` would have failed the build regardless of node.
Switch the workflow to `npm ci`, which installs the exact, known-good
versions already pinned in package-lock.json (select2 4.0.13, webpack
5.100.1) and is verified to build and lint cleanly. Pin the runner to node
24 via actions/setup-node so the toolchain is explicit rather than tracking
the runner default. Also pin select2 to ~4.0.13 in package.json so a future
`npm install` cannot pull the incompatible 4.1.0 back in.
Reproduction-style behavior test, not a regression test, so it does
not belong in the main suite. Full test remains in this branch's
commit history at 56c53ab9 for reviewers:
git checkout 56c53ab9 -- tests/Feature/QueueFailedHandlerTest.php
vendor/bin/phpunit --filter QueueFailedHandlerTest
Keeps the PR aligned with Heimdall's existing test conventions
(HTTP-feature tests, no facade-mocking unit tests).
Tests that UpdateApps::failed() and ProcessApps::failed():
- call Log::error with the 'permanently failed' message
- include exception_class, exception_message, and file context keys
- for UpdateApps, still call Cache::lock('updateApps')->forceRelease()
Uses Log::spy() and Mockery to capture facade calls. 2 tests, 4
assertions, green on PHP 8.4.20. See follow-up commit for why this
lands in history but not in the shipped suite.
Previously logged only the exception message. Adds:
- exception_class: distinguishes ClientException vs ConnectException
vs other Guzzle/PHP failure types at a glance
- file: file path and line where the exception was raised, useful
for distinguishing 'failed inside Guzzle' from 'failed inside our
code path'
The exception message itself often contains the GitHub API URL,
which encodes the app identifier. Capturing the specific appid at
the moment of failure would require touching handle() to track the
current iteration; left as a follow-up.
Most failures for these jobs are GitHub API rate-limit responses;
retries inside the same window do not help, so one attempt is enough
and $backoff has nothing to pace.
$timeout would clip the intentionally throttled handle() loop
(sleep(1) per app) below realistic workloads. Heavy users with 60+
apps would lose updates mid-cycle. The original code left $timeout
unset, and `UpdateApps` is dispatched via `dispatchAfterResponse()`
which doesn't go through `queue:work` at all (the queue $timeout
is irrelevant in that path). Letting the operator's worker config
govern is more honest.
$uniqueFor reduced to 600 (10 min) since with $tries=1 +
worker-governed timeout there is no long retry chain to outlive.
Lock self-heals 10 minutes after a crashed worker.
The test only asserted property values and method_exists. The diff
itself shows the property values; the test added zero confidence
beyond reading the diff. Heimdall is an app, not a package; its
existing tests are HTTP feature tests against user behavior, not
class-property assertions. Removing this file keeps the PR aligned
with the existing test conventions.
Both jobs implement ShouldBeUnique without a $uniqueFor value, which on
Redis and database drivers produces a lock that never expires. If the
worker is killed mid-fire (OOM, SIGKILL, server crash), the lock
persists and blocks all future dispatches of UpdateApps or ProcessApps
until the cache entry is manually cleared.
Neither job sets $tries, $backoff, or $timeout, so they inherit the
worker command's defaults (1 for queue:work, 0 for vapor:work), which
varies by platform and is brittle.
This change adds:
- $uniqueFor = 3600 lock expires after 1 hour
- $tries = 3 hard cap across worker restarts
- $backoff = [30, 60, 120] pace retries to reduce GitHub API load
- $timeout = 60 bound per-attempt wall-clock time
- failed(Throwable) log permanent failures (and preserve the
existing Cache::lock('updateApps')->forceRelease
on UpdateApps)
A new test (tests/Feature/QueueSafetyTest.php) asserts the retry
properties are present.
All existing tests still pass.
KodeStar
KodeStar
Fabien Ehrlich
Josh Salway
Chris Hunt
webmogul1
Adam
micvog