Skip to content

fix(repair): remove the oc_jobs rows the Cron → BackgroundJob move orphaned - #617

Merged
rubenvdlinde merged 7 commits into
developmentfrom
fix/remove-retired-cron-job-rows
Aug 27, 2026
Merged

fix(repair): remove the oc_jobs rows the Cron → BackgroundJob move orphaned#617
rubenvdlinde merged 7 commits into
developmentfrom
fix/remove-retired-cron-job-rows

Conversation

@rubenvdlinde

@rubenvdlinde rubenvdlinde commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Measured on a live instance, not inferred. After the equivalent opencatalogi move merged, that instance's oc_jobs still carried:

OCA\OpenCatalogi\Cron\DirectorySync
OCA\OpenCatalogi\Cron\RetentionEvaluation

next to their BackgroundJob replacements — rows naming classes that no longer exist. Found by querying the running Nextcloud, not by reading the diff.

Why the move alone could not do this

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 that is merely unavailable this boot. So the rename leaves the instance holding both rows.

The orphan is not inert: JobList cannot instantiate a missing class, so every cron tick that reaches the row fails to build it and logs rather than raises — the quiet kind of broken, on an instance where the replacement job runs fine and nothing looks wrong.

What the step does

Removes every class from the retired namespace, including any info.xml never registered: they lived in the same namespace, and an instance that ever registered one by hand carries the same dead row. Removing a registration that was never there costs nothing.

Idempotent — a fresh install removes nothing — and it never raises: a repair step that aborts trades a dormant job row for an instance that will not start. Registered post-migration only, since a fresh install has no rows to remove.

Tests

4 tests, 7 assertions, green. They assert that every removed name is in the retired Cron namespace and that no surviving BackgroundJob registration is ever touched — the failure mode of a future "remove anything matching Cron" edit. A further arm names the class this app actually registered, so the anti-widening arm cannot pass on a step that removes nothing at all; and one pins continue-after-failure.

Fleet context

Same fix landing across every app in this refactor: opencatalogi#1122, versioniq#231, and this one. Part of the 2026-08-25 fleet structure audit, ADR-100 Decision 3.

Conduction Release Bot
fix(repair): remove the oc_jobs rows the Cron -> BackgroundJob move o…
05b8f1e 
…rphaned

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

next to their BackgroundJob replacements — rows naming classes that no longer
exist. This app's move has the same shape and therefore the same gap.

WHY THE MOVE ALONE COULD NOT DO THIS. 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 the 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, on an instance where the replacement
job runs fine and nothing looks wrong.

Every class from the retired namespace is listed, including ones info.xml never
registered: they lived in the same namespace, and an instance that ever
registered one by hand carries the same dead row. Removing a registration that
was never there costs nothing.

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.
Registered post-migration only.

Tested: 4 tests, 7 assertions, green. They assert that every removed name is in
the retired Cron namespace and that NO surviving BackgroundJob registration is
ever touched — the failure mode of a future "remove anything matching Cron"
edit — plus one arm naming the class this app actually registered, so the
anti-widening arm cannot pass on a step that removes nothing at all, and one
pinning continue-after-failure.
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/learniq @ 1d21f6c

Check PHP Vue Security License Tests
lint ✅
phpcs ❌
phpmd ✅
psalm ✅
phpstan ❌
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 104/104
npm ✅ ✅ 650/650
app:check-code ⏭️
info.xml ✅
REUSE ❌
PHPUnit ✅
Newman ⏭️
Playwright ✅
Hydra gates ❌

Quality workflow — 2026-08-26 11:17 UTC

Download the full PDF report from the workflow artifacts.

Conduction Release Bot added 2 commits August 26, 2026 13:22
Conduction Release Bot
style(repair): put the file docblock above declare(), as phpcs requires
5d70b8b 
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.
Conduction Release Bot
fix(phpstan): annotate the one call that must pass a plain string
f37bce2 
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.
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/learniq @ 6556469

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ❌
psalm ❌
phpstan ❌
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 104/104
npm ✅ ✅ 650/650
app:check-code ⏭️
info.xml ✅
REUSE ❌
PHPUnit ❌
Newman ⏭️
Playwright ❌
Hydra gates ❌

Quality workflow — 2026-08-26 11:28 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/learniq @ c835e2f

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 104/104
npm ✅ ✅ 650/650
app:check-code ⏭️
info.xml ✅
REUSE ❌
PHPUnit ✅
Newman ⏭️
Playwright ❌
Hydra gates ✅

Quality workflow — 2026-08-26 12:02 UTC

Download the full PDF report from the workflow artifacts.

Conduction Release Bot added 2 commits August 26, 2026 14:43
Conduction Release Bot
fix(psalm): suppress the two findings a repair step necessarily produces
7bd7d93 
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!"
Conduction Release Bot
fix(annotations): split the phpstan and psalm suppressions apart
f37142a 
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.
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/learniq @ 252117a

Check PHP Vue Security License Tests
lint ✅
phpcs ❌
phpmd ✅
psalm ✅
phpstan ❌
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 104/104
npm ✅ ✅ 650/650
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.

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/learniq @ a5a53e7

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 104/104
npm ✅ ✅ 650/650
app:check-code ⏭️
info.xml ✅
REUSE ❌
PHPUnit ✅
Newman ⏭️
Playwright ❌
Hydra gates ❌

Quality workflow — 2026-08-26 14:07 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/learniq @ a6396d3

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 104/104
npm ✅ ✅ 650/650
app:check-code ⏭️
info.xml ✅
REUSE ❌
PHPUnit ✅
Newman ⏭️
Playwright ❌
Hydra gates ✅

Quality workflow — 2026-08-26 22:48 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit a033494 into development Aug 27, 2026
82 checks passed
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/learniq @ c2030c5

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
composer ✅ ✅ 104/104
npm ✅ ✅ 650/650
app:check-code ⏭️
info.xml ✅
REUSE ❌
PHPUnit ✅
Newman ⏭️
Playwright ✅
Hydra gates ✅

Quality workflow — 2026-08-27 00:24 UTC

Download the full PDF report from the workflow artifacts.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@rubenvdlinde