Repository navigation
Possible SAF cache bugs affecting hashing, deletion and directory lookups #263
Description
Hi, I asked AI to review BasicSync and run focused tests against commit 05062ab - I sometimes notices BasiSync eats more battery on S25U than before, so I tried a dig a bit deeper. Three things came up around the SAF cache. As I understand it, these could be addressed within BasicSync.
-
Cached metadata during hashing
safFile.Stat()uses cached metadata. In the test, a file grew from 6 to 15 bytes, but the actual scanner successfully hashed only the original 6 bytes. Another test changed the file during hashing, and the final check still accepted it.Expiring the metadata cache made both checks behave correctly. Could opened-file metadata be refreshed for these checks?
-
Removing a directory based on cached emptiness
Remove()can use a cached child listing.The test cached an empty directory, then added a file externally without a watcher notification. The walker missed that file, and removal called the mock provider’s recursive deletion, which deleted it. Refreshing the cache instead returned
ENOTEMPTYand preserved the file.Could directory removal require a fresh, complete listing?
-
Directory-map access and allocations
getChildren()returns a copy on cache hits, but returns the internal map after refreshing. Go’s race detector reported a race when that returned map was read during a locked removal.Also, individual
Resolve()lookups copy the whole directory map. Enumerating and resolving 1,000 entries allocated about 54.8 MB; 10,000 entries allocated about 4.37 GB in total, not peak memory.Would a locked single-child lookup and consistent snapshots address this?
These were Linux tests using Go 1.26.2, unchanged production code and a mocked Android provider. The existing SAF tests and the added control cases passed. Android behavior, complete peer synchronization and battery impact haven’t been verified.
Could you take a look whether these are valid bugs in the bridge?
Activity
chenxiaolong commentedon Sep 30, 2026
It is intentional that all cached metadata only expire either due to TTL or watcher notifications.
The SAF feature was added with AOSP's
FileSystemProvideras the main use case, which sends watcher notifications on file close (for the first case) and directory mutation (for the second case). Other third party SAF providers without watcher support will see filesystem state that doesn't match reality until TTL expiration. I don't really intend to support third party SAF providers, though the app currently doesn't prevent them from being used.That's definitely a bug! It's supposed to always return a clone. I'll fix that.
I might introduce a
getChild(childName)function to improve this so that copying the children map isn't necessary for path resolution. However, I don't plan on changing the caching or locking logic. SAF is painful enough that my main priority is just to keep it simple.stbridge: Fix safNode.getChildren() returning internal children map
stbridge: Reduce children map copying in safNode.resolve()
chenxiaolong commentedon Sep 30, 2026
The
getChildren()issue has been fixed in #264 and reducing children map copying inResolve()is done in #265.