fix(integrity): clear stale per app results and use the local cache tier

[DeepDiver1975]

Description

⚠️ Stacked on #41734 (base is perf/tier-host-local-caches). Review/merge that first; the base flips to master once the stack lands.

Fixes a real invalidation bug in the code integrity checker and moves its cache to the local tier. The tier move alone would be hard to justify, so it is paired with the bug that shares its root cause.

The bug: storeResults() writes one cache entry per checked scope — core, plus one per app — with no TTL (Checker.php:403), while cleanResults() removed only CACHE_KEY (:411-413). The per-app entries were therefore never invalidated, cluster-wide, forever. A verdict about an app was cached indefinitely and kept being served after the app was repaired or replaced. Those stale verdicts are read via getVerifiedAppsFromCache() into lib/private/legacy/app.php:808, settings/Controller/CheckSetupController.php:229-306 and lib/private/TemplateLayout.php:69.

The fix is one line: cleanResults() now calls $this->cache->clear() instead of remove(self::CACHE_KEY). ICache::clear($prefix = '') is @since 6.0.0 and prefix-scoped in every backend (Redis SCANs prefix*, APCu uses APCUIterator, ArrayCache truncates), so it drops the per-app keys and CACHE_KEY without touching other namespaces. This fixes local and distributed deployments.

Belt-and-braces: a CACHE_TTL (24 h) on the per-scope writes, so a node that never runs cleanResults() itself still notices a repaired installation.

The tier move: these results describe the files on disk of one host and are of no use to another — a second node consuming them is wrong even when nothing is malicious. __construct now uses LocalCacheFactory, keeping the existing null-factory branch.

Low-risk because getResults() already falls back to appconfig (:378-385), so occ integrity:check-core followed by a web request keeps working regardless of tier.

Flagged but deliberately NOT fixed here

storeResults():403 writes the whole $resultArray under the per-scope key, so getVerifiedAppsFromCache() returns a JSON string while the uncached verifyAppSignature() returns an array. app.php:808's empty($result) then yields a different level/removable depending on cache warmth. That is an observable-semantics change and CheckerTest.php:628-650 currently pins the string behaviour, so it belongs in its own PR.

Related Issue

• Fixes n/a — found while auditing distributed-cache usage for GHSA-488r-cjpq-p3vc

Motivation and Context

Per-app integrity verdicts were cached forever with no way to invalidate them, and they were shared between nodes that they do not describe.

How Has This Been Tested?

• test environment: PHP 8.3.32, sqlite, plus a run with memcache.local => APCu + memcache.distributed => Redis.
• test case 1: testRescanningDropsThePerAppResults() — seeds SomeApp, SomeOtherApp and CACHE_KEY into an ArrayCache, runs runInstanceVerification(), asserts the per-app keys are gone and CACHE_KEY is '[]'. This is the test that justifies the PR.
• test case 2: testUsesTheLocalCacheTier().
• test case 3: tests/lib/IntegrityCheck/CheckerTest.php builds a factory at six sites; all now use FixedCacheFactory, including the two that wrap createMock(Redis::class) and assert cache semantics (testVerifyCachedAppSignatureCheck, testAppNotCachedSignatureCheck) — so they keep asserting exactly what they asserted before. The prefix constraint that ->with('oc.integritycheck.checker') used to pin is now covered by getRequestedPrefixes(). 193 tests, 417 assertions, all green.
• test case 4: negative verification — both new tests confirmed to fail with the source change reverted, then restored.
• test case 5: real-instance — seeded two stale per-app verdicts into APCu, ran runInstanceVerification(), confirmed both cleared, redis KEYS *integritycheck* == 0, TTL present at 86400 s.
• test case 6: full tests/lib — 7149 tests, 40221 assertions, no new failures. make test-php-style and phan clean.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised: n/a
• Changelog item, see TEMPLATE

🤖 Generated with Claude Code

The base branch was changed.

[DeepDiver1975]

Code review

Verified against the PR head (b0c16fc670) in a clean worktree.

Verification performed

• tests/lib/IntegrityCheck/CheckerTest.php — 32 tests, 117 assertions, green
• tests/lib/UrlGeneratorTest.php + tests/lib/Files/Type/LoaderTest.php — 18 tests green
• php-cs-fixer on both changed files — 0 of 2 need fixing
• Negative verification: reverting clear() → remove(CACHE_KEY) fails testRescanningDropsThePerAppResults; reverting LocalCacheFactory::create → create() fails testUsesTheLocalCacheTier

The core bug and the one-line fix are real. clear() is prefix-scoped in every backend (Redis SCAN prefix*, APCu anchored APCUIterator regex, ArrayCache strpos===0, Memcached substr), so it does not leak into other namespaces.

---

1. occ integrity:check-core no longer invalidates the web nodes' verdicts

The description argues this is low-risk because "getResults() already falls back to appconfig". That fallback does not apply — getResults() reads the cache first and only consults appconfig on a miss:

$cachedResults = $this->cache->get(self::CACHE_KEY);
if ($cachedResults !== null) { return \json_decode($cachedResults, true); }
$v = $this->getAppValue(self::CACHE_KEY, '{}') ?? '{}';

A cache hit shadows the fresh appconfig value. Previously the tier was distributed, so occ's cleanResults() deleted the shared Redis key and every node then missed and re-read the repaired appconfig. Now occ clears only its own process-local tier — and on CLI that is usually nothing: APCu::isAvailable() returns false when apc.enable_cli is off, so Factory hands back a NullCache, which LocalCacheFactory replaces with a throwaway request-scoped ArrayCache. Confirmed directly: createLocal => NullCache, LocalCacheFactory::create => ArrayCache.

Net effect: after a repair, occ integrity:check-core leaves every FPM pool serving the stale verdict for up to 24 h. CACHE_TTL is therefore not belt-and-braces — it is the only thing carrying correctness. Defensible, but it is a behaviour regression for the documented operator workflow and should be stated as such.

Suggestion: have storeResults()/cleanResults() bump a cheap appconfig generation counter that getResults() compares against, so a pre-rescan cache entry is ignored rather than merely expiring.

2. cleanResults() on a Memcached local tier can flush the whole memcached instance

Memcached::clear() degrades to a full flush() when getAllKeys() fails — per the code's own comment, on "newer Memcached":

$allKeys = self::$cache->getAllKeys();
if ($allKeys === false) {
    // newer Memcached doesn't like getAllKeys(), flush everything
    self::$cache->flush();

cleanResults() passes no prefix, so this reaches every namespace on the node, including other ownCloud instances sharing the box (the global prefix is flushed too). This blast radius is new — the previous remove(self::CACHE_KEY) was a single-key delete. Reachable from the admin UI (GET /settings/integrity/rescan → rescanFailedIntegrityCheck()), occ, and Updater. '\OC\Memcache\Memcached' is a documented backend (config/config.sample.php:1142); the "take APCu for the local cache" line is advice, not enforcement.

Since only two known keys plus one per app need removing, an explicit remove() loop over ['core', ...$appIds] plus CACHE_KEY would fix the bug without inheriting clear()'s worst case.

3. CACHE_TTL has no test coverage

Stripping both self::CACHE_TTL arguments from storeResults() leaves all 32 tests passing. This is inconsistent with the sibling change in the same stack — LoaderTest::testCachedEntriesExpire does pin Loader::CACHE_TTL. Given finding 1 makes the TTL load-bearing rather than decorative, it deserves the same assertion.

Smaller notes

• Assertion inside setUp() (CheckerTest.php:105): assertSame([Checker::CACHE_KEY], ...getRequestedPrefixes()) fires on all 32 tests, so a break reports as a failure in every one rather than a single clear one. It also duplicates testUsesTheLocalCacheTier. Suggest dropping it.
• createMock(FixedCacheFactory::class) mocks a test double rather than an interface. Justified — the expects($this->never())->method('createDistributed') guard needs the concrete class — but worth a one-line comment, since it otherwise reads as an accident.
• testRescanningDropsThePerAppResults relies on getAllApps() returning [], so no app is actually re-verified. It proves clearing happens, not that the rescan repopulates per-app entries. Fine for the bug at hand.
• The deferred string-vs-array issue is good judgment to split out, but the tier move makes it worse in the interim: with a per-node cache plus a 24 h TTL, app.php:808's level/removable divergence now flips per node and per expiry window instead of being stable per installation. Argues for landing that follow-up promptly.
• Changelog follows changelog/TEMPLATE correctly.

Note on the diff

The stack base (#41734, #41753) has merged since this branch was pushed, so gh pr diff reports 12 files / 321 additions. All nine #41734 files are byte-identical to master — the real scope is 3 files: Checker.php, CheckerTest.php, changelog/unreleased/41735.

Verdict

The one-line invalidation fix is correct, well-motivated, and negatively verified. Findings 1 and 2 are worth follow-ups: the appconfig-fallback argument that makes the tier move look safe does not hold, and the clear() widening carries a full-flush worst case the old remove() did not.

🤖 Generated with Claude Code