fix(trashbin): avoid full filecache scan when restoring versions (#31682) by DeepDiver1975 · Pull Request #41641 · owncloud/core

DeepDiver1975 · 2026-06-19T15:01:49Z

Summary

Trashbin::getVersionsFromTrash() looks up a deleted file's versions with View::searchRaw('<name>.v%[.d<timestamp>]'), which reaches Cache::search() as:

WHERE `storage` = ? AND `name` ILIKE ?

The pattern is a leading literal followed by a trailing %, which is already non-sargable; on MySQL/MariaDB the ILIKE → COLLATE ..._general_ci LIKE rewrite (AdapterMySQL) additionally defeats any index on name. As a result every version restore — and every ExpireTrash / ExpireVersions cron run that restores versions — performs a full table scan of oc_filecache. On installations with a large filecache this is a heavy, repeated DB load spike. (Fixes #31682.)

Change

The version files for a given deleted file all live in a single, known directory under files_trashbin/versions (the folder the file was in, or the versions root). This PR replaces the whole-cache name search with an index-backed directory listing of that one directory (View::getDirectoryContent(), which uses the existing (parent, name) index) and filters the <name>.v<version>[.d<timestamp>] entries in PHP.

The directory to list is passed in from both call sites, so version files of files deleted from inside a folder are still found (the previous whole-cache search matched those by name regardless of location — the directory listing must look in the right sub-folder, not just the versions root).

Scope

No DB schema / db_structure.xml change.
No change to the generic Cache::search() / AdapterMySQL ILIKE behavior — the fix is contained to the trashbin version-restore hot path.

Behavior / compatibility

Returns the same versions, in the same shape, as before.
Literal PHP matching is slightly stricter than the prior SQL LIKE: filenames containing _ or % are no longer treated as wildcards (a latent edge-case bug in the old code), so if anything the result set is more correct, never broader.

Tests

Adds StorageTest::testGetVersionsFromTrash() covering:

the timestamped lookup in the versions root,
the non-timestamped lookup inside a sub-folder (the regression guard for nested deleted files),
and that version files belonging to other files / other deletion timestamps are correctly ignored.

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

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

`Trashbin::getVersionsFromTrash()` looked up a deleted file's versions with `View::searchRaw('<name>.v%[.d<timestamp>]')`, which reaches `Cache::search()` as `WHERE storage = ? AND name ILIKE ?`. The leading literal + trailing `%` is non-sargable, and on MySQL the `ILIKE`->`COLLATE` rewrite defeats any `name` index entirely, so every version restore — and every `ExpireTrash`/`ExpireVersions` cron run that restores versions — full scans `oc_filecache`. On large installations this is a heavy, repeated load spike (issue #31682). The version files for a given deleted file all live in a single known directory under `files_trashbin/versions` (the folder the file was in, or the versions root). Replace the whole-cache name search with an index-backed `getDirectoryContent()` of that one directory (uses the existing `(parent, name)` index) and filter the `<name>.v<version>[.d<timestamp>]` entries in PHP. The directory to list is now passed in from both call sites so version files of files deleted from inside a folder are still found. Behavior is preserved: the same versions are returned, in the same shape. Literal PHP matching is also slightly stricter than the previous SQL `LIKE` (filenames containing `_`/`%` are no longer treated as wildcards). Adds a regression test covering both the timestamped root lookup and the non-timestamped sub-folder lookup, and asserting that version files of other files / other timestamps are ignored. Fixes #31682 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>

update-docs · 2026-06-19T15:01:54Z

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

This PR fixes a real and well-diagnosed performance bug (#31682): Trashbin::getVersionsFromTrash() ran a non-sargable name ILIKE '<file>.v%[.d<ts>]' search that, combined with the MySQL ILIKE→COLLATE rewrite, full-scanned oc_filecache on every version restore / trash+version cron run. Replacing it with an index-backed getDirectoryContent() of the single known versions directory + PHP-side filtering is the right, appropriately-scoped approach (no schema change, no change to the generic Cache::search()).

The core fix looks correct, including the subtle part: passing $dir from both call sites so version files of files deleted from inside a sub-folder are still found (the old whole-cache search matched those by name regardless of location). 👍

🔴 Blocking — the new test will fail CI

In testGetVersionsFromTrash(), the first assertion is:

$versions = $method->invoke(null, 'test.txt', $timestamp, $this->user);
\sort($versions);
$this->assertEquals(['1', '10', '2'], $versions);

sort() defaults to SORT_REGULAR, which sorts the numeric strings '1','2','10' numerically → ['1', '2', '10']. But the test asserts ['1', '10', '2']. assertEquals compares sequential arrays element-by-element (no canonicalization by default), so this fails:

mismatch at [1]: expected '10', got '2'
mismatch at [2]: expected '2',  got '10'

Fix — make the expectation match numeric sort order:

$this->assertEquals(['1', '2', '10'], $versions);

(The second assertion, ['5', '6'], is fine.) An even more robust option is sort($versions, SORT_STRING) paired with a string-sorted expectation, or assertEqualsCanonicalizing() — but the minimal correct change is just reordering the expected array. Since the suite is @group DB it can't be run in the agent's sandbox, which is exactly why this slipped through; worth running locally/CI before merge.

Minor / non-blocking

$prefix matching is a substring-prefix, not a segment boundary. \strpos($name, $filename . '.v') !== 0 correctly anchors at the start, and the '.backup.v9' ignore-case in the test confirms a longer name isn't matched. Good. One residual edge: a sibling file literally named <filename>.vNN... where <filename> is a prefix of another real filename is already handled because you match the full $filename + .v. No action needed — just noting it was checked.
Comment/style: matches surrounding code (tabs, \-prefixed globals). Good.
Behavior note in the PR description is accurate: literal PHP matching is stricter than SQL LIKE for filenames containing _/%; that's a latent correctness improvement, not a regression.

Test coverage

Good instinct adding both the timestamped-root and non-timestamped-subfolder cases plus the "ignore other files/timestamps" entries — that subfolder case is precisely the regression risk of this change. Once the assertion above is corrected, coverage is solid. Consider also asserting the empty-result case (lookup for a filename with no versions) since getDirectoryContent on a non-existent dir returns [].

Verdict

Approach and core logic are correct; one blocking test bug (assertEquals ordering) must be fixed before this can pass CI. Everything else is minor.

The first assertion in testGetVersionsFromTrash() expected ['1', '10', '2'], but sort() uses SORT_REGULAR and sorts the numeric version strings numerically -> ['1', '2', '10'], so the test failed. Correct the expected order and add a case asserting an empty result for a file that has no stored versions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>

dj4oC

Replaces an unscoped wildcard filecache search (View::searchRaw()) with a folder-scoped View::getDirectoryContent() + PHP-side filtering in Trashbin::getVersionsFromTrash(), intended to avoid a full filecache scan when restoring/deleting versions (issue #31682).

Findings

Security: no findings — $dir is derived from the file's own trash path (not raw user input) and View is jailed per-user; no traversal risk. Correctly scoped as a CWE-400 (uncontrolled resource consumption) mitigation.
Stability: an earlier self-review claim that the test assertion (sort() on ['1','10','2']) would fail CI was independently checked and refuted — the actual assertion is ['1', '2', '10'], which is correct, and PHP's sort() under SORT_REGULAR does compare numeric strings numerically. Current CI is green (confirmed live, not from a stale claim).
Performance (blocking) — the stated goal may not hold in the dominant real-world case. View::getDirectoryContent() fully hydrates a FileInfo object (owner/share/mount-point resolution included) for every entry in the target directory before the new PHP loop discards non-matching ones. For files trashed directly at the trashbin root — arguably the most common case, and the case this method is called for per-file during bulk folder restores/deletes — $lookupDir resolves to the flat files_trashbin/versions root, which accumulates entries for every directly-trashed file for that user. Each call re-fetches and re-hydrates that entire directory from scratch with no caching/reuse across files in one bulk operation, which can be as expensive as (or worse than) the unscoped scan it replaces for users with large trash bins. Recommend caching the directory listing per unique $dir across a single bulk restore/delete operation, or using a cheaper indexed lookup that doesn't force full FileInfo hydration.
Test coverage: the added test never exercises $dir === '.' — the value dirname() actually returns for root-level trashed files, which is precisely why the code has a special ternary to normalize it. That's the dominant real-world call pattern and it's currently unverified by any test.

Verdict

Changes requested — not for a security or correctness bug (both refuted-vs-confirmed claims land in this PR's favor), but the performance goal isn't demonstrated for the common case, and the one branch written to handle that common case has no test.

🤖 Automated review by Claude Code (security · stability · performance · coverage)

Generated by Claude Code

jvillafanez · 2026-07-13T07:45:02Z

+		// The version files for a deleted file all live in a single, known
+		// directory under "files_trashbin/versions" (the folder the file was in,
+		// or the versions root). Instead of running a non-sargable
+		// trailing-wildcard name search across the whole filecache (full table
+		// scan on MySQL, see issue #31682), list that one directory using the
+		// index-backed (parent, name) lookup and filter the matching
+		// "<filename>.v<version>[.d<timestamp>]" entries in PHP.
+		$lookupDir = ($dir === '/' || $dir === '.') ? '' : \ltrim($dir, '/');
+		$prefix = $filename . '.v';
+
 		if ($timestamp) {
-			// fetch for old versions
-			$matches = $view->searchRaw($filename . '.v%.d' . $timestamp);
+			// match "<filename>.v<version>.d<timestamp>"
+			$suffix = '.d' . $timestamp;
 			$offset = -\strlen($timestamp) - 2;
 		} else {
-			$matches = $view->searchRaw($filename . '.v%');
+			// match "<filename>.v<version>"
+			$suffix = '';
+			$offset = 0;
 		}


It might be worthy to move this piece to a public helper. We can add specific unit tests for this part there.

jvillafanez · 2026-07-13T07:54:42Z

+			$name = $entry->getName();
+			if (\strpos($name, $prefix) !== 0) {
+				continue;
+			}
+			if ($suffix !== '') {
+				// must end with the deletion timestamp suffix
+				if (\substr($name, $offset) !== $suffix) {
+					continue;
 				}
+				$parts = \explode('.v', \substr($name, 0, $offset));
+			} else {
+				$parts = \explode('.v', $name);
 			}
+			$versions[] = (\end($parts));


We can try to move this to another function. A extractVersionFromVersionedFile looks better to describe what this code is doing.

DeepDiver1975 mentioned this pull request Jun 19, 2026

Sudden DB load peak caused by (apparent) old files cleanup. #31682

Open

DeepDiver1975 commented Jun 19, 2026

View reviewed changes

dj4oC suggested changes Jul 8, 2026

View reviewed changes

jvillafanez reviewed Jul 13, 2026

View reviewed changes

DeepDiver1975 marked this pull request as draft July 14, 2026 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(trashbin): avoid full filecache scan when restoring versions (#31682)#41641

fix(trashbin): avoid full filecache scan when restoring versions (#31682)#41641
DeepDiver1975 wants to merge 2 commits into
masterfrom
fix/issue-31682-trash-version-scan

DeepDiver1975 commented Jun 19, 2026

Uh oh!

update-docs Bot commented Jun 19, 2026

Uh oh!

DeepDiver1975 left a comment

Uh oh!

dj4oC left a comment

Uh oh!

jvillafanez Jul 13, 2026

Uh oh!

jvillafanez Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

DeepDiver1975 commented Jun 19, 2026

Summary

Change

Scope

Behavior / compatibility

Tests

Uh oh!

update-docs Bot commented Jun 19, 2026

Uh oh!

DeepDiver1975 left a comment

Choose a reason for hiding this comment

🤖 Automated code review by Claude Code review agent

Overview

🔴 Blocking — the new test will fail CI

Minor / non-blocking

Test coverage

Verdict

Uh oh!

dj4oC left a comment

Choose a reason for hiding this comment

Findings

Verdict

Uh oh!

jvillafanez Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

jvillafanez Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants