Trash Exclusion from Working Set Updates - #10752
Open
juliusvaart wants to merge 5 commits into
Open
Conversation
juliusvaart
requested review from
camilasan,
claucambra,
i2h3,
mgallien and
nilsding
as code owners
September 6, 2026 19:42
Collaborator
|
Please ensure this pull request is based on |
juliusvaart
force-pushed
the
macos/vf/trash-scan-exclusion
branch
from
September 7, 2026 13:39
35d1c9b to
50b6d78
Compare
Contributor
Author
|
Now based on |
juliusvaart
force-pushed
the
macos/vf/trash-scan-exclusion
branch
from
September 8, 2026 06:29
50b6d78 to
e19820f
Compare
A folder created on the Mac is, from the framework's point of view, already fully enumerated: it knows the empty listing it just created and will not ask for the contents again. The refresh subscription a browsed folder gets must therefore be granted here too — `visitedDirectory` is what puts a directory into the materialised set the working-set scan reads, and `downloaded` is ignored for directories there. Without it a locally created folder is never PROPFINDed by the scan, so items added to it on the server (web UI, public upload link, another user) never surface, and no later `enumerateItems` exists to repair that. Resolves: nextcloud#9688 Signed-off-by: Julius van der Vaart <julius@vanderva.art> Assisted-by: Claude Code:claude-opus-5
Browsing into a folder that had never been enumerated showed an empty view and a spinner that never resolved. Finder's status bar read "0 items" for a folder holding three subfolders on the server, and the framework never scheduled a fetch for its children -- it logged a new enumerator and then asked it for nothing at all, not even a sync anchor, while the extension sat idle. `childItemCount` derived the number from the local database, which holds a directory's children only once something has read that directory. For one nobody had read it therefore returned 0. But `NSFileProviderItem.childItemCount` is optional precisely so a provider can say it does not know, and 0 is not "I do not know", it is "this folder is empty". Finder printed that verbatim, and the framework, told the container had no children, had no reason to ask for any. Report nil whenever the local set is empty. There is no flag available to distinguish a genuinely empty directory from an unread one -- `visitedDirectory` is still false after a paginated enumeration, which is the path every server from Nextcloud 31 takes -- so an empty local set cannot support the stronger claim. The cost is one enumeration of a truly empty folder, which the framework performs on first browse anyway. The cost of the previous answer was the folder never being enumerated at all. `delete` had been reading nil as "empty or a file", which was safe only while nil meant a file. It now asks whether the item is a directory, so a directory whose contents are unknown refuses a non-recursive delete rather than assuming it has nothing to lose. Two existing tests asserted the old 0. Their expectations move to nil, with the reason recorded at the assertion. Signed-off-by: Julius van der Vaart <julius@vanderva.art> Assisted-by: Claude Code:claude-opus-5
`visitedDirectory` marks a directory as actually read. Two things depend on it: membership of the working set the remote-change scan walks, and whether an empty child count means "this folder is empty" or "nobody has looked". The flag is local-only, so it takes no part in `isInSameDatabaseStoreableRemoteState`. The depth-1 ingestion decided whether to persist its read target purely on that comparison, so reading a directory whose remote state had not changed persisted nothing and dropped the visit. That is not an edge case: a directory's etag does not change because somebody opened it, so the ordinary browse recorded nothing. The paginated ingestion path already handled this -- `addItemMetadataPreservingLocalState` compares the flag explicitly -- so only the non-paginated path was affected. Record the visit separately rather than by adding the row to `metadatasToUpdate`. That set is also the change set handed to the framework, and announcing a directory as changed every time it is first browsed would re-queue its update-item job for a flag the framework cannot see. Signed-off-by: Julius van der Vaart <julius@vanderva.art> Assisted-by: Claude Code:claude-opus-5
Covers the path a notify_push or root-ETag signal actually drives: `enumerateChanges(.workingSet)` -> `scanMaterialisedItemsForRemoteChanges()` -> `pendingWorkingSetChanges(since:)` -> the change observer. Each test mutates the mock server and asserts the change reaches `MockChangeObserver`. Three reproduced real silent drops in the enumerator and database change derivation, and now guard them: the scan returning the changes it discovers rather than relying on the lossy syncTime reconstruction, recursion into changed subdirectories, and `size` participating in the change-detection predicate. Signed-off-by: Julius van der Vaart <julius@vanderva.art> Assisted-by: Claude Code:claude-opus-5
Trashing rewrites an item's `serverUrl` to the trashbin but leaves `deleted` false, and a folder keeps `visitedDirectory`, so a trashed folder stayed in the materialised set and the scan PROPFINDed it through the ordinary DAV path. That 404s, and the scan reads a 404 as "the item is gone": it reported the item deleted and hard-removed the very row the trash reconciliation derives permanent deletions from. Trash has its own enumeration path via `listingTrashAsync`. The regression test fails on both counts without the fix — it observes the PROPFIND to /remote.php/dav/trashbin/... and the row's destruction. Signed-off-by: Julius van der Vaart <julius@vanderva.art> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code:claude-opus-5
juliusvaart
force-pushed
the
macos/vf/trash-scan-exclusion
branch
from
September 8, 2026 06:49
e19820f to
a908860
Compare
Collaborator
|
If I read correctly, this depends on #10751. So let's wait until that is integrated first. |
i2h3
changed the title
Contributor
|
Artifact containing the AppImage: nextcloud-appimage-pr-10752.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
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
2 participants
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.
Summary
A folder in the trash was still being scanned as if it were a live folder, and
the 404 that produced destroyed the database row that trash restore and
permanent-deletion reconciliation depend on.
Trashing rewrites an item's
serverUrlto the trashbin path but leavesdeletedfalse, and a folder keeps itsvisitedDirectoryflag. Both are whatput an item in the materialised set, so a trashed folder stayed in it and the
working-set scan PROPFINDed it through the ordinary DAV path. That path 404s for
something that now lives in the trashbin — and the scan reads a 404 as "the item
is gone". It reported the item deleted and hard-removed the row, which is the
same row the trash reconciliation derives permanent deletions from. Trash has
its own enumeration path via
listingTrashAsyncand does not need the scan tovisit it.
The fix excludes trashed rows from the scan's seed set.
Changes
notify_push or root-ETag signal actually drives:
enumerateChanges(.workingSet)→scanMaterialisedItemsForRemoteChanges()→
pendingWorkingSetChanges(since:)→ the change observer. Each test mutatesthe mock server and asserts the change reaches
MockChangeObserver. Three ofthem reproduce silent drops that were real: the scan relying on the lossy
syncTimereconstruction instead of returning what it discovered, recursioninto changed subdirectories, and
sizeparticipating in the change-detectionpredicate.
materialised set the scan walks.
Testing
Full suite green at 359 XCTest + 59 Swift Testing; the first commit is green
standalone at 358 + 59.
testTrashedFolderIsNotScannedThroughTheRegularDavPathfails on both countswithout the fix: it observes the PROPFIND going to
/remote.php/dav/trashbin/…and it observes the row being destroyed.Depends on #
This branch is stacked on the directory-visits PR and its diff here includes
those three commits until that one merges.
The dependency is real, not just packaging.
testItemCreatedOnServerInLocallyCreatedFolderIsReportedfails onstable-34.0without "mark locally created directories as visited", and the trashed-rows
change does not apply to the test file without the tests commit that precedes
it. Happy to rebase and repush once the other one lands.
Checklist
AI (if applicable)