Fix change-watcher lifecycle and mode-switch handling - #114
Conversation
in-jun
left a comment
There was a problem hiding this comment.
Reviewed against the four referenced issues — all four are resolved and the changes map cleanly onto them with no scope creep.
#20 (RemoteStorage lifecycle) — remoteChanges correctly owns the instance and closes it in finally. Confirmed only remote() needs this: local() returns a plain Storage (not AutoCloseable), so leaving it unwrapped is right. I also checked that SmbRemoteStorage.close() acts on the constructor's main client, which is a distinct SMBClient from the watch's own secureClient()/watchConn (torn down in changes()'s awaitClose), so completing or cancelling this flow can't prematurely kill a live watch and there's no double-close.
#16 (mode switch) — Deriving the watched set from combine(settings, pairs) with distinctUntilChanged + flatMapLatest is the right shape. REALTIME collapses to emptyList (foreground service takes over), PERIODIC restarts immediately, and since SyncPair/AppSettings are data classes the distinctUntilChanged meaningfully avoids restarting the watch on unrelated settings edits.
#13 (directory watcher) — Good handling. Replace-not-skip fixes the delete+recreate inode case, recursive observeTree on CREATE/MOVED_TO covers bulk moves, and the parent-side releaseTree prefix sweep is robust even if individual child *_SELF events are dropped under queue pressure.
#99 — flows.merge() is the clean equivalent.
One minor, non-blocking observation: deleting or moving the root itself releases only the root's own entry via *_SELF, so descendant map entries then rely on their individual DELETE_SELF events rather than a parent-side releaseTree sweep — under inotify overflow a few could linger. It's an unusual case (the whole sync root vanishing while watched, where the watch is effectively broken anyway and the safety poll covers correctness), and it's strictly better than the prior always-leak behavior, so I'd leave it as-is for now.
LGTM.
Cleans up the realtime change-watching subsystem: leaked resources, stale watchers, and a redundant spread.
RemoteStorageinstances created by the change stream.ChangeTriggers.forPairsdiscarded eachRemoteStorageright after calling.changes(), ignoring itsAutoCloseablecontract. It now creates the instance inside aflow { ... }that closes it infinally, so a backend whose watch reuses the main connection can't leak one per resubscription.MainViewModel.onForegroundedreadsettings.modeonce, so switching PERIODIC↔REALTIME left the in-app watcher running alongside the foreground service's watcher (duplicate subscriptions) or stopped live sync until the next ON_RESUME. It now derives the watched pairs fromcombine(settings, pairs), tearing down or starting the watch immediately on a mode change.DirectoryWatcher's observer map only ever grew: noDELETE_SELFin the mask, and re-registration was skipped for a recreated path, so a delete+recreate went unwatched. It now handlesDELETE_SELF/MOVE_SELFand parent-sideDELETE/MOVED_FROMto drop stale entries, attaches observers recursively on bothCREATEandMOVED_TO, and replaces rather than skips an existing entry.Iterable<Flow<T>>.merge()instead ofmerge(*flows.toTypedArray()), dropping the array allocation and spread.Fixes #99
Fixes #20
Fixes #16
Fixes #13