feat(sync): record and show what each sync pass actually did - #285
Conversation
SyncStatus was a timestamp and a boolean, so a pass that transferred a million events and one that transferred nothing both rendered as "succeeded" (ActivityWatch#274). aw-sync returns a SyncReport across the JNI boundary now (ActivityWatch/aw-server-rust#699); the app was parsing only success/message/error out of it and discarding the aggregates. - SyncStatus carries events pulled/pushed, peers imported/skipped/failed, and warnings, with a hasReport marker so a run recorded by an older native lib still renders as before rather than as "pulled 0, pushed 0". - SyncStatus.fromJniResponse parses the payload in one place and never throws; a missing error field no longer propagates an exception. - Status persistence moved into performSyncAsync, the single choke point all four sync entry points pass through. Only the full-sync path used to record a status, so manual pull/push runs left the settings line stale. - formatSyncStatus appends the facts line and any warnings. Bumps the aw-server-rust submodule to pick up #699 (plus #705, whose port parameter the Kotlin declaration already matches, and #704 docs only). Git-Session-Id: 4b0fca5c-13ca-53f7-9910-98381b800dd3
|
Model matches what #274 asked for — events pulled/pushed, peers imported/skipped/failed, warnings, parsed from the One consequence worth stating explicitly: this PR bumps the Merge on green (Android cadence, independent of the desktop cut). |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // through here, and this is the only place that holds the SyncReport | ||
| // returned across the JNI boundary. Persisting per-caller (as the full-sync | ||
| // path used to) is what left pull/push runs unrecorded. | ||
| AWPreferences(appContext).setLastSyncStatus(status) |
There was a problem hiding this comment.
For a full sync with a configured SAF directory, this persists and broadcasts the native success before mirroring finishes. The settings UI can therefore report a successful completed sync while required mirroring is still running. If mirroring then fails, the catch path replaces the result with a report-less failure status, losing the counts and warnings from the completed native sync. Persist the final status after mirroring and retain the parsed report when recording a mirror failure.
There was a problem hiding this comment.
Fixed in a226ec7 — the first half of this finding was indeed still live: the success status (with its SyncReport) was persisted before mirroring ran, so the settings UI could show a completed full sync while the SAF mirror was still in flight (the earlier c6cced7 fix only addressed the report-retention half).
performSyncAsync now persists after mirrorSyncFilesToSafDir() returns for mirroring operations (success && mirrorBeforeCallback); non-mirroring operations keep the immediate persist, so pull/push runs stay recorded at the single choke point. The catch path is unchanged: a mirror failure persists the native report carried into a failure status.
|
One pin detail before this merges: the submodule bump is That matters here because #678 is the v0.14.x stopgap for #253 (owner-originated event edits not reaching the phone). Pinning Not blocking — if you'd rather keep this PR's bump minimal and fold #678 into the next pin, that's fine, but it's worth deciding deliberately rather than by pin arithmetic. No code change made from my side. |
🤖 AI code reviewThis PR adds SyncReport fields (events pulled/pushed, peer counts, warnings, hasReport) to SyncStatus, parses them from the JNI response via a new fromJniResponse() function, persists them in AWPreferences, and renders them in the sync settings UI. It also moves status persistence into performSyncAsync so all sync operations record a status, and bumps the aw-server-rust submodule. Not safe to merge — 1 P1 openConfidence 3/5 1 finding · ❌ 1 P1❌ P1 high — In the catch path of performSyncAsync, when the native sync succeeded but a post-sync step (SAF mirroring or callback) throws, the code builds a failure status via native.copy(...) and then calls AWPreferences.setLastSyncStatus(status). However, the original native status had success=true and hasReport=true, and the copy keeps hasReport=true while setting success=false. The settings UI then renders the failure headline followed by the detail line (counts/warnings). That is intended. But there is a subtler issue: the catch block also runs when the exception is thrown by AWPreferences.setLastSyncStatus(status) itself on line 289 (e.g., a SharedPreferences failure or a broadcast exception). In that case nativeStatus is non-null and native.success is true, so the catch builds a status with error 'post-sync step failed: ...' and calls setLastSyncStatus again, which may throw again, propagating out of the executor thread and never invoking the callback. The callback is only invoked inside the try after setLastSyncStatus, so a failure in persistence leaves the caller hanging without a callback. This is a real error-handling gap: the callback is not guaranteed to be called if setLastSyncStatus throws. The consequence is that the sync operation appears to hang from the caller's perspective (e.g., the UI never gets the success/failure callback), and the status may be partially persisted. The fix is to wrap the persistence call in its own try/catch or move it after the callback, or ensure the callback is always invoked in a finally-like manner. How this was verified: Traced the try/catch in performSyncAsync: the callback is posted only after setLastSyncStatus on line 290. If setLastSyncStatus throws, control jumps to catch, which calls setLastSyncStatus again (line 309) and then posts the callback with the failure status. If the second setLastSyncStatus also throws, the callback is never posted. Even if the second succeeds, the callback is posted with a failure status, but the original success is lost. The caller (e.g., syncBothAsync) relies on the callback to clear syncInFlight; if the callback never runs, syncInFlight stays true and all future syncs are skipped. 1 advisory finding (summary-only, not scored)These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.
The new fromJniResponse function uses json.optInt for counts and coerces to at least 0. However, if the JSON contains a very large number (e.g., 2^31-1), optInt returns that value, and the UI displays it. If the JSON contains a number larger than Int.MAX_VALUE, optInt returns the default 0? Actually, org.json.JSONObject.optInt returns the default if the value is not an integer or if it overflows? Let me check: JSONObject.optInt uses Number.intValue() which truncates. For a long value like 3000000000, it would return a negative number due to overflow, and then coerceAtLeast(0) would turn it into 0. That could under-report. But the native lib is unlikely to return counts that large. This is a guard-level concern. How this was verified: org.json's optInt uses Number.intValue() which can overflow for large longs. For a value > 2^31-1, it becomes negative and is coerced to 0, under-reporting. This is a theoretical edge case. Files changed (5) — the diff as I read it
Previous review passes
Reviewed Maintainer commands
|
The success status (with SyncReport counts/warnings) is persisted before SAF mirroring runs for full syncs. If mirroring then threw, the catch path replaced the persisted status with a report-less failure, erasing what the pass actually did. Keep the native status and record the mirror failure on top of it, preserving counts and warnings. Git-Session-Id: 029e7f5e-0490-56c7-ad5f-5d0a05d90ea0
|
@greptileai review |
|
Fixed the P1 from Greptile (c6cced7): a full sync persisted the success status (with its SyncReport counts/warnings) before SAF mirroring, and a mirroring failure overwrote it with a report-less failure status — erasing what the native pass actually did.
CI should pick this up; re-review triggered. |
Pins the submodule at #678 (reconcile owner-originated event edits), which landed six minutes after the previous pin 37aa0b0 and is directly relevant to the sync reporting this PR consumes. Git-Session-Id: 38fdae10-a8ad-5df2-bf53-99533d65f74c
|
@greptileai review |
|
Bumped the |
Greptile P1 (thread 4034960583): for a full sync with a configured SAF directory, the success status was persisted before mirroring finished, so the settings UI could show a completed sync while the mirror was still running. The persist now happens after the mirror for mirroring operations; non-mirroring operations keep the immediate persist. The catch-path report retention is unchanged. Git-Session-Id: 93c0a634-35df-50c7-a1d8-36d5fa94d03c
|
@greptileai review |
|
CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click. This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted. |
|
Addressed the remaining Greptile P1 on the new head
|
…rvive A payload whose first five warning entries normalize to blank would drop any later meaningful warning. Filter blanks first, then cap. Git-Session-Id: 3794dab5-f699-5115-ade7-bcdd99be743d
…er counts Git-Session-Id: 3794dab5-f699-5115-ade7-bcdd99be743d
|
Addressed the two findings from the latest AI-review round on head
|
| if (success && mirrorBeforeCallback) { | ||
| mirrorSyncFilesToSafDir() | ||
| } | ||
| AWPreferences(appContext).setLastSyncStatus(status) |
There was a problem hiding this comment.
❌ P1 — In the catch path of performSyncAsync, when the native sync succeeded but a post-sync step (SAF mirroring or callback) throws, the code builds a failure status via native.copy(...) and then calls AWPreferences.setLastSyncStatus(status). However, the original native status had success=true and hasReport=true, and the copy keeps hasReport=true while setting success=false. The settings UI then renders the failure headline followed by the detail line (counts/warnings). That is intended. But there is a subtler issue: the catch block also runs when the exception is thrown by AWPreferences.setLastSyncStatus(status) itself on line 289 (e.g., a SharedPreferences failure or a broadcast exception). In that case nativeStatus is non-null and native.success is true, so the catch builds a status with error 'post-sync step failed: ...' and calls setLastSyncStatus again, which may throw again, propagating out of the executor thread and never invoking the callback. The callback is only invoked inside the try after setLastSyncStatus, so a failure in persistence leaves the caller hanging without a callback. This is a real error-handling gap: the callback is not guaranteed to be called if setLastSyncStatus throws. The consequence is that the sync operation appears to hang from the caller's perspective (e.g., the UI never gets the success/failure callback), and the status may be partially persisted. The fix is to wrap the persistence call in its own try/catch or move it after the callback, or ensure the callback is always invoked in a finally-like manner.
There was a problem hiding this comment.
Fixed in ff3472f.
Both persist calls in performSyncAsync now go through a persistSyncStatus() helper that logs and swallows a storage/broadcast failure, so the completion callback is posted even if setLastSyncStatus throws. That closes the exact path you traced: previously an exception from line 289 fell into the catch, the catch persisted again (line 309, which could throw a second time), and only then posted the callback — so a persistence failure could either suppress the callback entirely or relabel a successful sync as failed.
Two things this preserves that are worth stating:
- A status that could not be stored is no longer treated as a sync that failed — the failure is logged, and the callback reports the real native outcome.
- The callback is what clears
syncInFlightinsyncBothAsync, so the change also removes the "one persistence failure and every later sync is rejected as already in flight" lockup.
Verified: ANDROID_HOME=... ./gradlew :mobile:clean :mobile:testStandardDebugUnitTest --tests "net.activitywatch.android.SyncSettingsActivityTest" → BUILD SUCCESSFUL.
performSyncAsync posted its completion callback only after AWPreferences.setLastSyncStatus() returned. If persistence threw — in the try path or, worse, in the catch path's own persist — the callback was never posted. Callers use it to clear syncInFlight, so a single persistence failure would leave every later sync rejected as "already in flight" and the UI waiting. Route both persist calls through persistSyncStatus(), which logs and swallows the failure. A sync whose status could not be stored is not a failed sync, and the callback contract must not depend on storage health. Git-Session-Id: 6b9bee26-4b2b-533a-bb4e-97220cf3780c
The marker decides whether the renderer appends the counts line. Including `warnings` in the key list let a payload that carried warnings but no count fields mark itself as report-bearing, so formatSyncStatus would render "pulled 0, pushed 0" — inventing a no-op pass out of a report that never supplied numbers. A pass that reports counts still marks itself through those keys, so push-only responses (events_pushed + peer counts, no events_pulled) are unaffected. Git-Session-Id: 6b9bee26-4b2b-533a-bb4e-97220cf3780c
|
Addressed the P1 from the latest AI-review round, plus the two items a local re-review raised on
Verified locally: Greptile was 5/5 on the prior head; neither change touches anything Greptile flagged, so no re-trigger. CI is running on the new head. |
2 participants
Problem
SyncStatusis a timestamp and a boolean, so a pass that transferred a millionevents and one that transferred nothing both render as
Last sync succeeded at ….That is the same silence that hid ActivityWatch/aw-server-rust#682 on desktop for
months (#274).
aw-sync now returns and persists a
SyncReportfrom every pass(ActivityWatch/aw-server-rust#699, merged), and that report already crosses the
JNI boundary —
to_jni_json()is what all four sync entry points return. TheAndroid side was reading only
success/message/errorfrom it and throwing therest away.
What changed
SyncStatuscarries the report aggregates: events pulled/pushed, peersimported/skipped/failed, and warnings, plus a
hasReportmarker. The countsdefault to
0, which is indistinguishable from a real no-op pass, so therenderer keys off the marker rather than the values.
SyncStatus.fromJniResponseparses the JNI payload in one place. It neverthrows: an unreadable response becomes a failure carrying the raw text, and a
missing
errorfield no longer propagates an exception out of the sync path.Persistence moved into
performSyncAsync— the single choke point everysync operation passes through, and the only place holding the parsed report.
Previously only the full-sync path recorded a status, so manual pull and push
runs left the settings line stale.
formatSyncStatusappends a facts line:Warnings render below it. When the payload carried no report the output is
byte-for-byte what it was before, so an older bundled native lib renders
unchanged.
Submodule bump:
aw-server-rust97f51e3→37aa0b0for #699 (theSyncReport). The bump also picks up #705 (
startServerport parameter — theKotlin declaration on master already passes an
Intport, verified againstJava_net_activitywatch_android_RustInterface_startServer) and #704 (docs only).Verification
no-op success that must not read as a plain success, peer outcomes, warnings
(capped and whitespace-normalized), negative counts, a failure payload, and an
unreadable response.
Not in this PR
SyncSchedulerquestion; no report data isinvolved, so it does not belong in the status model.
carries
peers[].hostnameand an outcome tag; surfacing them is a naturalfollow-up once the aggregate line proves useful.
Part of #274 — the model now records what synced. Still open there: the next-attempt
and per-peer detail.