fix: StreamingService foreground crash, import crash, and sync change-detection bugs#406
fix: StreamingService foreground crash, import crash, and sync change-detection bugs#4067MS8 wants to merge 7 commits into
Conversation
On GrapheneOS (and strictly enforced Android 12+), the 5-second window for calling startForeground() was being missed because StreamingService extended IntentService, which ran startForeground() on a background worker thread with non-deterministic scheduling delay. Convert StreamingService from deprecated IntentService to Service: - Call startForeground() in onStartCommand() on the main thread, guaranteeing foreground promotion before any background-process limits can apply - Run the blocking waitFor() in a dedicated thread - Return START_NOT_STICKY (user-initiated serve, no auto-restart) - Add tryStartForegroundService() to ActivityHelper and use it for all StreamingService start calls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
getJSONArray() throws JSONException when the key is absent. Backups from older versions only contain the arrays that exist — filters in particular was added later and is missing from all pre-existing exports. Replace with optJSONArray() and return an empty list when the key is not present, so a partial backup still imports the sections that are there. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
log2File was declared but never assigned, so log2File?.log() was always a no-op even when logging was enabled. Initialize it in doWork() when logging is enabled. Also add null guard in Log2File for cases where getExternalFilesDir() returns null (e.g. on GrapheneOS). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Update: Added Fix 3 — External log file was never written.
Commit: a746cab |
rclone was running at default log level (WARNING), so INFO messages like per-file operations and "There was nothing to transfer" were never visible. Added --log-level INFO to sync command and collect rclone INFO lines into a single SyncLog entry per sync run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
For SFTP remotes, add --sftp-md5sum-command none and --sftp-sha1sum-command none to prevent rclone from calling md5sum via SSH on the remote host. On Synology NAS with restricted shells, md5sum cannot access paths exposed only via SFTP, causing all transfers to fail with "corrupted on transfer" errors. Also fixes a silent startup crash: --log-level INFO conflicts with -vvv (added when debug logging is enabled), causing rclone to exit immediately with "Can't set -v and --log-level". Now --log-level and --stats-log-level are only added when verbose logging is off. Additionally capture NOTICE-level rclone output (transfer stats) and include it in the in-app sync log alongside INFO messages, and guard postSync() mTask accesses with isInitialized checks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Combining --checksum with --sftp-md5sum-command/--sftp-sha1sum-command "none" (set for SFTP remotes since the previous fix to avoid "corrupted on transfer" errors on restricted shells) leaves rclone with no common hash type to compare. rclone silently falls back to a size-only comparison and ignores modification time, so changed files of the same size never get re-synced. Skip --checksum for SFTP remotes so rclone uses its default size+modtime comparison instead. Verified against a live Synology SFTP remote: a same-size, content-changed file is now correctly flagged via modtime and re-transferred, where before it was silently skipped. Note: a separate, longer-standing issue also causes silent size-only comparisons independent of this fix -- RCLONE_LOCAL_NO_SET_MODTIME=true (set for every rclone invocation since 2021, commit f43949b, to avoid chtimes crashes per rclone#2446) makes rclone treat the local backend's modtime as unreliable, which disables modtime-based comparison globally. This fix does not address that; tracked separately.
…MODTIME RCLONE_LOCAL_NO_SET_MODTIME=true has been set for every rclone invocation since 2021 (f43949b) to avoid chtimes crashes on old Android storage stacks (rclone#2446). It has a much bigger side effect than intended: rclone's local backend Precision() reports fs.ModTimeNotSupported unconditionally whenever NoSetModTime is set (backend/local/local.go), and rclone's default file comparison treats ModTimeNotSupported as "always equal" as soon as sizes match (fs/operations/operations.go) -- skipping modification-time comparison entirely. Every sync, regardless of remote type or --checksum, silently degraded to a same-size-means-unchanged check. Changed files that happened to keep the same size were never re-synced. Verified in current rclone (v1.71.0, vendored in this repo) that a failing chtimes()/SetModTime is not fatal: fs/operations/operations.go just logs and counts the error, the transfer itself still succeeds. The 2021 workaround is no longer needed and was silently breaking change detection instead. Verified against a live Synology SFTP remote: a same-size, content-changed file is now correctly detected via modtime and re-transferred ("Copied (replaced existing)"), a second sync correctly no-ops once the file is genuinely in sync, and the local destination's mtime is now correctly set to match the source with no chtimes errors logged.
|
@7MS8 thanks for the fixes! I have a question though.
Can you explain your thoughts behind this change? Have you considered any other solutions for this issue? I am probably not convinced that disabling it for all SFTP remotes is a good idea but I didn't go too deep here so that's why I am asking.
I can confirm this bug - not sure if it's reported (haven't found such issue opened). I have just tested an import from a ZIP that was exported from 2.5.6 fdroid installation and I only got remotes populated but no tasks were imported. It also showed an error but probably a wrong one (something about tasks and triggers missing in the archive - most likely caused by missing filters though). @newhinton by any chance - will you have any capacity to look at this PR and maybe provide a bit of feedback? Should it be split into more PRs? Which of these fixes are good to be merged and which ones would you want to be discussed? |
|
Thanks both for taking a look, and Peter for the review! On disabling The original problem (fixed in 9926e3f): Synology's SFTP-only restricted shell has no full SSH shell access — That fix introduced a second, more subtle bug (fixed in 8b62460): with the remote hash commands disabled, combining that with Alternatives I considered:
Happy to follow up with a per-remote toggle if that's the preferred direction — let me know and I can open a follow-up PR instead of changing the SFTP default further here. On the import crash (Fix 2): thanks for confirming the repro. What you're seeing ("remotes populated but no tasks imported") is explained precisely by the bug: @newhinton whenever you have a chance, feedback on scope would help — happy to split this into separate PRs per fix if that's easier to review than one bundled diff. |
|
@7MS8 Nothing against using AI to help write or refine a message, but please consider other people’s time as equally valuable as your own. Nobody enjoys reading that much AI-generated text. The less effort you put into editing and condensing the real valuable information in the text, the more effort everyone else has to spend parsing it.
Do you really stand behind this statement? Why would detecting it require one failed transfer per file? In general:
One final point: if you keep feeding my replies back into AI and sending me another generated response, I don’t think this discussion will get very far. I think a good starting point would be to split the unrelated fixes into separate PRs and take them step by step. For smaller PRs that do not already have a corresponding issue, please also spend a bit more time explaining how to reproduce the particular problem so others can review and test your solution too if they have the capacity. |
|
Hi Peter, Fair point, and sorry — I don't have the technical background to defend the specifics or dig into the scope/reproducer questions myself. I just wanted the app to sync reliably with my NAS, and Claude Code did the actual fix over about half a day; I only ran the tests and managed the process. So I can't really engage with your questions on the merits. If it helps in the meantime, you can find a built APK with these fixes on my fork's releases (7MS8/Round-Sync, tag I'll split this PR into separate, smaller PRs per your suggestion so each fix can be reviewed on its own. Will follow up here once they're up, and close this one out. Thanks a lot for taking the time to review this so carefully. Michael |
|
Split into 6 separate PRs, one per fix, each with its own reproduction steps:
Closing this one in favor of those. Thanks again for pushing for this — happy to keep discussing the SFTP hash-verification tradeoff on #413. |
Problem
Six bugs fixed in this PR, found and fixed while getting Round-Sync working reliably on GrapheneOS against a Synology SFTP remote.
Fix 1: StreamingService killed immediately on GrapheneOS
On GrapheneOS (and any Android variant with strict foreground service enforcement), the streaming / file-browser feature does not work. The
rclone serve httpprocess starts but is killed within 2–30 seconds before the WebView can load the file listing.Root cause:
StreamingServiceextended the deprecatedIntentService.startForeground()was called from a background worker thread (onHandleIntent), not from the main thread inonStartCommand. Android requiresstartForeground()within 5 seconds ofstartForegroundService(). On GrapheneOS this window was consistently missed.Fix:
StreamingServicefromIntentServicetoServicestartForeground()immediately inonStartCommand()on the main threadwaitFor()in a dedicated thread; returnSTART_NOT_STICKYtryStartForegroundService()toActivityHelper; use it for allStreamingServicestart callsFix 2: Settings import crashes on backups without a
filtersarrayImporter.createFilterList()calledreader.getJSONArray("filters")which throwsJSONExceptionwhen the key is absent. Thefiltersarray was added later and is missing from all pre-existing exports (including the current F-Droid release v2.5.6). This exception aborted the entire import — rclone.conf was already written but tasks and preferences were silently skipped.Fix: Replace
getJSONArray()withoptJSONArray()+ null-check forfilters,tasks, andtriggerarrays inImporter.java.Fix 3:
log2Filenever initialized inSyncWorker/EphemeralWorkerFollow-up to Fix 2 — after fixing import, sync logging (
log2File) was never initialized in these two workers, so sync log entries were silently dropped in some code paths.Fix 4: rclone INFO output not visible in the in-app sync log
rclone's own progress/status output (transferred bytes, errors, "nothing to do", etc.) wasn't being captured into the in-app sync log, making it hard to diagnose sync issues from within the app. Now captured at INFO level.
Fix 5: SFTP sync fails on restricted shells +
-vvv/--log-levelconflictTwo related SFTP/logging issues:
md5sum/sha1sumover SSH even though the files are reachable via SFTP, causingcorrupted on transfer: hashes differfailures on every transfer. Fixed by setting--sftp-md5sum-command none --sftp-sha1sum-command nonefor SFTP remotes.-vvvwas being combined with--log-level INFO, which rclone rejects (Can't set -v and --log-level), causing rclone to exit immediately with no useful output.Fix 6: Changed files silently skipped by sync — two compounding bugs
Reported symptom: sync appeared to detect changes "only by file size, not by date" — files with the same size that had genuinely changed content were never re-synced.
Bug 6a: Fix 5's
--sftp-md5sum-command nonecombined with--checksum(set when "use checksum" is enabled on a task) left rclone with no common hash type to compare. rclone silently fell back to a size-only comparison. Fixed by not setting--checksumfor SFTP remotes — rclone then uses its default size+modtime comparison instead.Bug 6b (the actual root cause):
RCLONE_LOCAL_NO_SET_MODTIME=truehas been set for every rclone invocation since 2021 (to avoid oldchtimescrashes, ref rclone#2446). In current rclone this makes the local backend'sPrecision()reportModTimeNotSupportedunconditionally, which makes rclone's default comparison treat any same-size file as unchanged — skipping modification-time comparison entirely, for every task regardless of remote type or checksum setting. Verified in the vendored rclone v1.71.0 source that a failingchtimes()/SetModTimeis not fatal (just logged and counted, transfer still succeeds), so the 2021 workaround is no longer needed and was silently breaking change detection. Removed from bothRclone.javaandRcloneRcd.java.Both 6a and 6b were verified against a live Synology SFTP remote: a same-size, content-changed file is now correctly detected via modtime and re-transferred, a second sync correctly no-ops once genuinely in sync, and the local destination's mtime is now correctly set to match the source with no
chtimeserrors logged.Test plan
🤖 Generated with Claude Code