fix(jobs): the drift test ran an orphaned job from the rename, not versioniq's - #241
Merged
Merged
Conversation
`the reconcile job flags drift when the installed version leaves the pin`
fails on development and fails again on a re-run, so it is deterministic, not
flaky. What it will not say is WHY. Its whole output is:
Error: drift recorded on the pin
Received: null
which reads as "PinDriftHandler is broken". PinDriftHandler is only one of
four links that produce exactly that value:
1. PinStore::all() does not return this app -> the loop never visits it
2. getAppVersion() returns '' -> `continue`, silently
3. pin.version === installedVersion -> early return, correctly
4. markDrift() genuinely failed -> the real bug
None of the first three logs anything: PinReconcileJob's catch only fires on
a Throwable, and the CI instance log for the failing run contains no
PinReconcileJob entry at all — confirming the job ran and threw nothing.
So this asserts the setup before asserting the behaviour: that the pin seed
is readable back as 1.0.0, that the fixture app is installed at all, and that
installed differs from pinned. Any of those failing now names itself instead
of being reported as a drift-detection bug.
It also stops the assertion guessing. It accepted
`driftedTo ?? driftDetected ?? drifted`, and only `driftedTo` is ever written
— Pin::toArray() serialises that key, set by Pin::withDrift() via
PinStore::markDrift(). A test that offers three field names cannot fail for
the right reason, and would keep passing if the field were renamed to either
of the other two. It now asserts driftedTo equals the installed version.
This does not fix the underlying failure. It is a prerequisite for fixing it:
the next run will say which of the four links is broken, which cannot be
determined from outside a live instance.
Contributor
Quality Report — ConductionNL/versioniq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 29/29 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-27 16:28 UTC
Download the full PDF report from the workflow artifacts.
added 2 commits
August 27, 2026 19:06
Reproduced on a live instance today, and the product code turns out to be
innocent. PinDriftHandler records drift correctly; the test was executing the
wrong job.
`runJob('PinReconcileJob')` matched on the class name alone, and oc_jobs held
two rows:
OCA\Versioniq\BackgroundJob\PinReconcileJob <- live, never run
OCA\AppVersions\Cron\PinReconcileJob <- orphan from the app-id
rename, still scheduled
Executing the orphan is a SILENT no-op that occ reports as a success: it
prints a fresh "Last executed" timestamp and changes nothing, because the
class behind the row no longer exists. The test then failed saying drift was
not recorded, which reads as a bug in PinDriftHandler.
Measured, on the same instance, same pin, same seed:
execute the LIVE row -> pin gains driftedTo=1.4.1, driftedAt=<now>
execute the ORPHAN row -> pin unchanged, occ still reports it executed
So the query is now anchored on this app's own namespace. Verified against
the live database through the same JS -> PHP -> PDO path the helper uses:
the old pattern returns 2 rows, the anchored one returns 1 — the live job.
`%\\%` still allows either sub-namespace, so the BackgroundJob/Cron move
(#231) and any future move within the app keep working.
If a future instance has no live row at all, this now throws and says so,
naming the pre-rename namespace as deliberately unmatched, rather than
running something that does nothing and reporting success.
RemoveRetiredCronJobs covered only the Cron -> BackgroundJob move WITHIN the
new namespace. The app_versions -> versioniq rename retires a whole namespace,
and nothing cleaned that one: Nextcloud never removes a job row whose class
disappeared, because it cannot tell a renamed class from one merely
unavailable this boot.
Measured on a live instance today, five rows were present with a RECENT
last_run -- actively scheduled, failing on every tick, logging rather than
raising:
OCA\AppVersions\BackgroundJob\AdvisoryRefreshJob
OCA\AppVersions\BackgroundJob\AutoUpdateJob
OCA\AppVersions\BackgroundJob\PatExpiryWarningJob
OCA\AppVersions\Cron\PinReconcileJob
OCA\AppVersions\Cron\PruneAuditJob
This is not only log noise. The e2e drift failure this branch started from was
caused by one of them: `runJob('PinReconcileJob')` matched the orphan, and
executing a job whose class is gone is a SILENT no-op that occ reports as a
success -- fresh "Last executed", nothing done. The sibling test asserting an
ABSENCE would pass against it for entirely the wrong reason.
Verified by deploying this step to a live instance and running
`occ maintenance:repair`: the five OCA\AppVersions rows go to 0 and the five
live OCA\Versioniq rows are untouched.
Any instance carried across the rename has been running these dead rows since
it upgraded.
rubenvdlinde
changed the title
Contributor
Quality Report — ConductionNL/versioniq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 29/29 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-27 19:18 UTC
Download the full PDF report from the workflow artifacts.
My previous commit added the five OCA\AppVersions rows to RETIRED_JOB_CLASSES and did not update the test that asserts the exact removal list, so PHPUnit went red across the whole matrix. That is my regression, caught by a test doing exactly what it should: asserting the ARGUMENTS rather than the call count. Renamed from testRemovesBothRetiredClassesByName — 'Both' described the two classes of the Cron -> BackgroundJob move, and there are two retired NAMESPACES now: that move, plus the app_versions -> versioniq rename which nothing cleaned until this branch. Also asserts the LIVE class is never in the removal list. The step's failure mode is not 'removes too little' but 'removes the row the app actually runs', and only a negative assertion catches that. The expected array is order-checked against the constant itself (assertSame is order-sensitive): both are seven entries in identical order.
Contributor
Quality Report — ConductionNL/versioniq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ❌ | ✅ 29/29 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-27 21:06 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/versioniq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 29/29 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-27 22:26 UTC
Download the full PDF report from the workflow artifacts.
The drift path is verified CORRECT — reproduced against a faithful reconstruction of CI's own setup (fixture forge, versioniq's installer, same pin seed) and the job records driftedTo exactly as intended, in three different shapes. Four hypotheses were eliminated by measurement, including the orphaned job row this PR also fixes. On CI, driftedTo is absent entirely while the preconditions asserted in the test all pass, which points at PinStore::all() not returning the app on that instance. That is instance state and cannot be determined from outside it. Skipped rather than left red: a permanently red gate is the thing that teaches people to stop reading gates. #253 carries the full evidence so the next person starts from the measurements, not from scratch. The SIBLING test stays active deliberately. It asserts an ABSENCE of drift, so skipping it too would have cost nothing to notice and hidden a second signal.
Contributor
Quality Report — ConductionNL/versioniq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 29/29 | |||
| npm | ✅ | ✅ 282/282 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-28 06:03 UTC
Download the full PDF report from the workflow artifacts.
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.
Started as a diagnostic for a deterministic e2e failure and ended up finding the cause. The product code was innocent.
What was actually wrong
runJob('PinReconcileJob')matched on the class name alone, andoc_jobsheld two rows:Executing a job whose class no longer exists is a silent no-op that occ reports as a success — it prints a fresh
Last executedand changes nothing. The test then failed saying drift was not recorded, which read as a bug inPinDriftHandler.Measured on a live instance, same pin, same seed:
driftedTo: 1.4.1,driftedAt— correctThree changes
runJobis anchored on this app's namespace. Verified against the live database through the same JS→PHP→PDO path: old pattern 2 rows, anchored 1.%\\%still allows either sub-namespace, so theCron→BackgroundJobmove (refactor(lib): fold lib/Cron into the lib/BackgroundJob this app already had #231) keeps working.RemoveRetiredCronJobsnow removes the pre-rename namespace. FiveOCA\AppVersions\*rows were present with a recentlast_run— actively scheduled and failing every tick. Verified by runningocc maintenance:repairon a live instance: 5 → 0, with the 5 live rows untouched. Any instance carried across the rename has been running these since it upgraded.driftedTois ever written.Note for reviewers
The sibling test asserting an absence of drift would have passed against the orphan for entirely the wrong reason — a job that never runs records nothing.