fix(backgroundjob): don't log ERROR for stale job rows of removed apps (#35589)

[DeepDiver1975]

Summary

Fixes #35589 — the background-job runner logged an ERROR-level stack trace on every cron run for stale job rows whose class no longer exists (the "SyncJob does not exist" log spam).

JobList::buildJob() caught the QueryException from \OC::$server->query($row['class']) and always called logException(..., ERROR) before checking whether the class even exists. A stale oc_jobs row left behind by a removed or disabled app therefore produced a recurring ERROR-level stack trace, even though the job was correctly skipped (return null).

Change

Check class_exists() first inside the catch:

• Class exists but failed to resolve as a service → genuine, actionable DI failure → still logged at ERROR via logException() (unchanged).
• Class no longer exists (stale row from a removed/disabled app or old version) → logged once at DEBUG with a concise message, then skipped.

Control flow and return values are unchanged (new $class() fallback when the class exists; return null for stale rows).

Tests

Updates the existing JobListTest::testUnknownJobLogsException → testUnknownJobDoesNotLogException: a missing-class job must not call logException and instead logs at debug. (The old test asserted the now-removed ERROR behavior.)

Tagged @group DB (existing class). php -l clean; full PHPUnit not run in the preparation environment.

Note: owncloud/core is in maintenance mode; targets installations on classic ownCloud 10.x.

---

🤖 This PR was prepared by the Claude Code review agent from the analysis of #35589. Please review carefully before merging.

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

🤖 Automated code review by Claude Code review agent

Overview

Fixes #35589 — the cron background-job runner logged an ERROR-level stack trace on every run for stale oc_jobs rows whose class no longer exists (removed/disabled apps). The QueryException catch in JobList::buildJob() is restructured to check class_exists() first: a class that exists but fails DI resolution still logs at ERROR via logException(); a class that no longer exists logs once at DEBUG and returns null. The existing test is renamed testUnknownJobLogsException → testUnknownJobDoesNotLogException to assert the new behavior. +19/-4 across 2 files, targeting master.

Correctness

• Control flow / return values unchanged. The class-exists branch is byte-for-byte equivalent (logException then new $class()); the missing-class branch still return null. The only behavioral change is downgrading the missing-class log from ERROR (always) to DEBUG (missing only) — exactly the intended fix. The outer AutoloadNotAllowedException handler is untouched. Correct.
• DEBUG log call is valid. $this->logger is typed ILogger (constructor param). ILogger::debug($message, array $context = []) — the two-argument call debug('Background job class ... skipping', ['app' => 'core']) matches the signature.
• No silent failure introduced. Genuine DI failures (class present, service unresolvable) keep their ERROR-level logException, so actionable failures remain visible. Only the benign stale-row case is quieted. Good.
• Minor (non-blocking): the message embeds raw $row['class'] from the DB. For a fix targeting log spam this is fine and consistent with surrounding code, just noting the value is attacker-uncontrolled-but-DB-sourced.

Tests

• addWrongJob() inserts the class literal 'wrong job title'. listJobs() selects all rows and calls buildJob() on each, so the stale row reaches the catch: \OC::$server->query('wrong job title') throws QueryException, class_exists('wrong job title') is false (contains a space — no such class), so the DEBUG branch fires and null is returned. Path confirmed.
• Assertions match the implementation precisely: logException → never() (the whole point of the fix); debug → atLeastOnce() with stringContains('wrong job title') (the message contains $row['class'] = 'wrong job title') and exact context ['app' => 'core'] (matches the code). The test will PASS.
• No other test in the file asserts the old ERROR/logException behavior — the single prior occurrence is the renamed test. No stale assertion is left to break.
• atLeastOnce() (rather than once()) is the right choice — it tolerates other rows without over-constraining.

Caveat: tagged @group DB and full PHPUnit was not run in the prep environment (per PR description). The logic and mock expectations check out by inspection, but a real DB test run is the final confirmation.

Verdict

approve-with-nits. The fix is correct, minimal, behavior-preserving except for the intended ERROR→DEBUG downgrade, and the rewritten test accurately reflects and would pass against the new code. No blocking issues. The only outstanding item is running the @group DB PHPUnit suite to confirm green, which CI should cover.

[phil-davis]

@jvillafanez @DeepDiver1975 is there some way to get Claude to create a changelog file when it creates a PR?

Otherwise, we keep having to push an extra commit with the changelog, and then CI runs all over again.