refactor(lib): fold lib/Cron into the lib/BackgroundJob this app already had - #231
Merged
Merged
Conversation
added 2 commits
August 26, 2026 11:07
…ady had This app carried BOTH directories: three jobs in lib/BackgroundJob/ and two in lib/Cron/, with info.xml registering from each. Nextcloud's convention is BackgroundJob/; Cron/ is a directory the app framework has no notion of. Part of the 2026-08-25 fleet structure audit (ADR-100 Decision 3), which found two apps in this split state. PinReconcileJob and PruneAuditJob move, with their namespaces, @Package tags and both unit tests (whose own namespace was Tests\Unit\Cron — a second string that does not match the class namespace). AppUpdatedListener and PinDriftHandler referenced them and move too. TWO THINGS WOULD HAVE BROKEN SILENTLY, not one: 1. appinfo/info.xml registers by fully-qualified class name. A move without it leaves the registration pointing at a class that no longer exists, and Nextcloud does not fail the install for that — the job just never runs, which looks exactly like a job that ran and found nothing to do. 2. tests/phpunit-unit-only.xml listed as a test directory and as a coverage source. Moving the files without it would have left PHPUnit collecting from a directory that no longer exists: the two tests stop running and the suite still reports green, because a testsuite that matches nothing is not an error. Both entries are removed; the BackgroundJob entries already present now cover the moved files, verified by counting the collected files. Verified: all five registered classes resolve, both XML files parse, every touched PHP file passes php -l, and no stale Cron reference remains.
…rphans MEASURED ON A LIVE INSTANCE, not inferred. After the equivalent move merged for opencatalogi, that instance's oc_jobs still carried OCA\OpenCatalogi\Cron\DirectorySync OCA\OpenCatalogi\Cron\RetentionEvaluation alongside their BackgroundJob replacements — classes that no longer exist. WHY THE MOVE ALONE IS NOT ENOUGH. appinfo/info.xml's <job> entries are a REGISTRATION instruction, not a description of state. On upgrade Nextcloud ADDS any job it does not already have; it never removes one whose class disappeared, because it cannot distinguish a renamed class from one merely unavailable this boot. So a rename leaves the instance holding BOTH rows. The orphan is not inert: JobList cannot instantiate a class that does not exist, so every cron tick reaching that row fails to build it and logs rather than raises — the quiet kind of broken. It also breaks anything resolving a job by NAME: this app's own e2e helper looks jobs up with `class LIKE '%PinReconcileJob%' LIMIT 1`, which with two matching rows and no ordering can return the dead one and execute nothing. RemoveRetiredCronJobs removes both retired classes, is idempotent (a fresh install removes nothing), and never raises — a repair step that aborts trades a dormant job row for an instance that will not start. TWO PRE-EXISTING DEFECTS SURFACED WHILE TESTING THIS, both of the same shape: 1. tests/unit/Repair was NOT in the testsuite list in phpunit-unit-only.xml, so both tests under it had never run. A testsuite matching no directory is not an error — it collects nothing and reports green. 2. Once collected, all 7 of the existing MigrateSchemaApplicationIdTest cases errored with `Class "Doctrine\DBAL\ParameterType" not found`. OCP's IQueryBuilder derives its PARAM_* constants from that class at class-definition time, and doctrine/dbal belongs to the SERVER, not to an app. tests/stubs/server-internals.php existed for exactly this purpose and was never required by bootstrap-unit-only.php. Both are fixed here: the directory is collected, the stub file is loaded, and ParameterType / ArrayParameterType are added to it. Unit suite: 575 tests, 1194 assertions, all green — up from 565 collected before, the difference being the 10 repair tests that had never run.
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-26 10:11 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-26 11:11 UTC
Download the full PDF report from the workflow artifacts.
added 2 commits
August 26, 2026 13:22
phpcs failed with one blocking error:
4 | ERROR | Inline doc block comments are not allowed;
| use "/* Comment */" or "// Comment" instead
A docblock that FOLLOWS declare(strict_types=1) is an inline docblock, not a
file docblock. Every existing lib/Repair/*.php in this repo puts it first; the
header now matches, with the same @category/@Package tags.
Worth recording how this slipped through: I checked the convention by reading
line 2 of a sibling file, which is BLANK — the docblock starts at line 3. The
detector reported "declare-first" for a repo that is docblock-first.
And I verified the fix with `phpcs | grep -c "| ERROR"` against COLOURED
output, where the ANSI escapes sit between the pipe and the word, so the
pattern never matched and a failing file read as clean. Both readings are now
taken with `sed "s/\x1b\[[0-9;]*m//g"` first.
Also adds reasoned @SPEC exclusions on the public methods: exclusions rather
than links because no capability spec covers this move — ADR-100 Decision 3 is
an architecture record, and the jobs behaviour is unchanged, only where their
classes live.
phpstan: Parameter #1 $job of method OCP\BackgroundJob\IJobList::remove() expects class-string<OCP\BackgroundJob\IJob>|OCP\BackgroundJob\IJob, string given. The narrow type is correct for the callers it was written for — code REGISTERING a job has the class. This step RETIRES one, and the class is gone by construction: that is the entire reason the oc_jobs row has to be removed. A class-string is unobtainable here, and remove() only uses the value as the `class` column to delete on. Annotated at the call site rather than added to a baseline, so the reason travels with the code instead of living in a file nobody reads. Verified with phpstan on the file: [OK] No errors. phpcs still reports 0 blocking errors.
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-26 11:29 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-26 11:43 UTC
Download the full PDF report from the workflow artifacts.
added 2 commits
August 26, 2026 14:43
psalm reported both on this file:
UnusedClass: Class ...\Repair\RemoveRetiredCronJobs is never used
ArgumentTypeCoercion: Argument 1 of IJobList::remove expects
IJob|class-string<IJob>, but ...Cron\PinReconcileJob provided
Both are correct readings of code psalm cannot fully see.
UnusedClass: Nextcloud instantiates repair steps from the <repair-steps> block
in appinfo/info.xml. That is XML; psalm reads PHP, so it sees no caller. Worth
naming why the sibling steps do not trip this — unrelated docblocks happen to
{@see} them, which is a coincidence, not a contract. The suppression sits on
the CLASS docblock, the only place psalm reads it for this issue; my first
attempt put it in a floating docblock inside the class body, where it
suppresses nothing.
ArgumentTypeCoercion: the same tension phpstan flagged. remove() is typed for
callers REGISTERING a job, which have the class. This step RETIRES one, and the
class is gone by construction — that is the entire reason the row must be
removed.
Both suppressions carry their reason inline rather than going into a baseline,
so a future reader sees why rather than finding an unexplained entry.
Verified with psalm on the file: "No errors found!"
Combining both tags in one docblock before the call broke two tools at once: phpcs Inline doc block comments are not allowed phpstan Parse error in @PHPStan-Ignore: Unexpected T_OTHER "@psalm-suppress" phpstan reads everything after `@phpstan-ignore` on the following lines as part of its ignore expression, so the psalm tag became a parse error rather than being skipped. And phpcs forbids a `/** */` docblock before a statement at all, which is the only form psalm accepts for a statement-level suppression. The two tools want incompatible shapes in the same place, so they go to different places: - phpstan as a LINE comment at the call site (`// @PHPStan-Ignore ...`), which phpcs permits. - psalm on run()`s own docblock, a legal docblock position, with the reason written out there. Verified all four on the same file: phpcs 0 errors, phpstan [OK] No errors, psalm "No errors found!", and the unit suite green.
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-26 12:49 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-26 13:03 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 27, 2026
`runJob()` read `if (id) { … }`, so a missing `oc_jobs` row made the helper a
silent no-op. The job never ran, the test carried on, and the assertion that
followed failed with whatever the job should have produced. On
jobs.spec.ts:169 that surfaces as "drift recorded on the pin" — which reads
as a product bug in PinDriftHandler when it may simply mean PinReconcileJob
was never executed. Those need different fixes, so they must not wear the
same words.
The sibling test is worse off: `the reconcile job records no drift while the
installed version matches the pin` asserts an ABSENCE, so a job that never
runs makes it PASS. A silent no-op cannot fail that test at all.
The row can genuinely go missing. This app has moved its job classes twice --
`lib/Cron` into `lib/BackgroundJob` (#231) and the app_versions -> versioniq
rename (#187) -- and each move orphans the rows registered under the old
class string. RemoveRetiredCronJobs clears `OCA\Versioniq\Cron\*`; nothing
clears an `OCA\AppVersions\…` leftover.
So this also asserts the match is UNIQUE. `LIKE '%…%' LIMIT 1` with no ORDER
BY picks an arbitrary row, and an orphan sitting beside the live job could be
executed instead -- silently, because executing a job whose class no longer
exists does nothing observable.
This does not by itself fix the failing drift test: it makes that test say
which of the two things went wrong. Diagnosing the drift failure needs a live
instance, and the message it prints today points at the wrong layer.
Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
rubenvdlinde
pushed a commit
that referenced
this pull request
Aug 27, 2026
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.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 28, 2026
…rsioniq's (#241) * test(e2e): make the drift test name which link broke `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. * fix(e2e): run THIS app's job, not a same-named orphan from the rename 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. * fix(repair): remove the job rows the app_versions rename orphaned 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. * test(repair): the retired list is seven classes now, not two 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. * test(e2e): skip the CI-only drift failure, citing #253 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. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
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
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.
This app carried both directories — three jobs in
lib/BackgroundJob/, two inlib/Cron/, withinfo.xmlregistering from each. Nextcloud's convention isBackgroundJob/;Cron/is a directory the app framework has no notion of. Part of the 2026-08-25 fleet structure audit (ADR-100 Decision 3).PinReconcileJobandPruneAuditJobmove with their namespaces,@packagetags and both unit tests — whose own namespace wasTests\Unit\Cron, a second string that does not match the class namespace.AppUpdatedListenerandPinDriftHandlerreferenced them and move too.A class rename orphans its
oc_jobsrow — measured, not inferredAfter the equivalent move merged for opencatalogi, that live instance's
oc_jobsstill held:beside their
BackgroundJobreplacements — naming classes that no longer exist.info.xml's<job>entries are a registration instruction, not a description of state. On upgrade Nextcloud adds any job it does not have; it never removes one whose class disappeared, because it cannot tell a renamed class from one merely unavailable this boot. So the rename leaves the instance holding both.The orphan is not inert:
JobListcannot instantiate a missing class, so every cron tick reaching that row fails to build it and logs rather than raises. It also breaks anything resolving a job by name — this app's own e2e helper usesclass LIKE '%PinReconcileJob%' LIMIT 1, which with two matching rows and no ordering can return the dead one and execute nothing.RemoveRetiredCronJobsremoves both retired classes. Idempotent (a fresh install removes nothing) and never raises — a repair step that aborts would trade a dormant row for an instance that will not start.Two pre-existing defects surfaced while testing this
Both the same shape — a test that never ran:
tests/unit/Repairwas missing from the testsuite list inphpunit-unit-only.xml, so both tests under it had never run. A testsuite matching no directory is not an error; it collects nothing and reports green. My new test's first run saidNo tests executed!, which is how this was found.Once collected, all 7 existing
MigrateSchemaApplicationIdTestcases errored withClass "Doctrine\DBAL\ParameterType" not found. OCP'sIQueryBuilderderives itsPARAM_*constants from that class at class-definition time, and doctrine/dbal belongs to the server, not to an app.tests/stubs/server-internals.phpexisted for exactly this purpose and was never required bybootstrap-unit-only.php.Both fixed here: the directory is collected, the stub file is loaded, and
ParameterType/ArrayParameterTypeare added to it.Verification
php -l.info.xml's<job>andphpunit-unit-only.xml's directories both verified against the post-move tree.