Skip to content

feat(sync): automatic one-shot staging reset (#296) - #328

Open
TimeToBuildBob wants to merge 3 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/reset-staging-one-shot
Open

TimeToBuildBob wants to merge 3 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/reset-staging-one-shot

Conversation

@TimeToBuildBob

@TimeToBuildBob TimeToBuildBob commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Kotlin half of #296: an automatic one-shot reset of the phone's push staging db, so pre-ActivityWatch/aw-server-rust#713 duplicate copies stop being re-imported by desktops. Item 2 of ActivityWatch/aw-server-rust#717 (item 1, desktop dedupe, merged in ActivityWatch/aw-server-rust#718).

What it does

  • StagingReset.runOnce (Android-free, unit-tested): skips if this hostname's flag is set; otherwise calls the native reset and sets the flag only on {"success": true}. Failures leave the flag unset, so the reset retries on the next sync. It never throws, including UnsatisfiedLinkError.
  • SyncInterface: declares external fun resetStaging(hostname), and runs the reset inside the full-sync executor task, after the legacy-folder migration and before syncBoth. That path holds syncInFlight, so the reset cannot race a push writing the same staging db. (The push/pull-all wrappers have no callers and no guard, so they are left alone.)
  • AWPreferences.isStagingResetDone/setStagingResetDone(hostname): the flag is keyed per hostname because staging lives under {sync_dir}/{hostname}/.

Dependency: the Rust side is not written yet

resetStaging must be added to aw-sync/src/android.rs (spec in #296: delete test.db/-wal/-shm for the own device dir only, refuse paths outside it). Until a build carries it, the native call throws UnsatisfiedLinkError. That error is caught and logged, the flag stays unset, and sync carries on, so this PR is safe to merge before the Rust PR. The reset then fires on the first sync after the submodule bump.

Verify before closing #296

  • Confirm the phone's local android-test / android-unlock buckets are not duplicated. If they are, re-staging reproduces the copies.
  • After the Rust side ships: a pull on the desktop peer should add zero events.

Relates to #296, ActivityWatch/aw-server-rust#717, ActivityWatch/aw-server-rust#713, #295.

@greptile-apps

greptile-apps Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Adds automatic staging database reset to sync flow.

The Android changes appear safe to merge before the native reset ships.

What we checked:

  • Missing native method stops sync: runOnce catches LinkageError and returns Failed. The caller logs that result, then continues to syncBoth.

Summary

Adds Android-side support for a one-time staging reset before full sync.

  • Saves completion separately for each hostname, only after the native call reports success.
  • Catches a missing native method so sync can continue and retry the reset later.
  • Replaces the broken Mockito tests with direct tests of StagingReset.runOnce.
  • The three earlier, unnumbered findings are addressed: preference methods now exist, Mockito imports are gone, and the replacement tests exercise the reset logic.
  • The native reset still requires a separate Rust change. No new actionable issues were found.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Full sync takes shared guard] --> B[Migrate legacy folders]
    B --> C{Reset done for hostname?}
    C -- Yes --> G[Run native full sync]
    C -- No --> D[Call native reset]
    D --> E{Reset reports success?}
    E -- Yes --> F[Save completion flag]
    E -- No or missing method --> G
    F --> G
    G --> H[Report sync result and release guard]
Loading

Reviews (2) · Last reviewed commit: "fix(sync): make staging reset compile, t..."

Comment thread mobile/src/main/java/net/activitywatch/android/SyncInterface.kt Outdated
Comment thread mobile/src/test/java/net/activitywatch/android/SyncInterfaceStagingResetTest.kt Outdated
Comment thread mobile/src/test/java/net/activitywatch/android/SyncInterfaceStagingResetTest.kt Outdated
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Ready for maintainer review: all three Greptile P1s are fixed in e0bcfec. The re-review on that head scores 5/5, CI is green (Test + E2E), and every thread is resolved.

I can't merge this one myself (pull-only access on aw-android), so it's waiting on a maintainer.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

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.

@TimeToBuildBob

TimeToBuildBob commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 AI code review

Safe to merge — no P0/P1 findings on latest review

Updated after inline dispositions on finding threads — this is the current state; the verdict below is frozen at review time and is kept as the historical record of that pass.

Finding disposition
Finding Severity State
mobile/src/main/java/net/activitywatch/android/StagingReset.kt:43 P1 accepted-tradeoff
mobile/src/main/java/net/activitywatch/android/SyncInterface.kt:334 P1 superseded by latest review (not reproduced)
mobile/src/main/java/net/activitywatch/android/AWPreferences.kt:78 P2 rejected
mobile/src/main/java/net/activitywatch/android/StagingReset.kt:15 P2 superseded by latest review (not reproduced)
mobile/src/main/java/net/activitywatch/android/StagingReset.kt:45 P2 accepted-tradeoff
mobile/src/main/java/net/activitywatch/android/StagingReset.kt:15 P2 rejected
mobile/src/main/java/net/activitywatch/android/SyncInterface.kt:334 P2 accepted-tradeoff
mobile/src/test/java/net/activitywatch/android/StagingResetTest.kt:68 P2 accepted-tradeoff

Adds a one-shot staging-db reset for the full-sync path: a new StagingReset object (Android-free, unit-tested) runs a native resetStaging(hostname) call once per hostname, marking a SharedPreferences flag only on a JSON success response, and SyncInterface invokes it inside the syncBoth executor task before syncBoth. AWPreferences gains per-hostname getter/setter for the flag, and a new test file covers the runOnce logic.

Needs a look — P2 only

Confidence 4/5

1 finding · ⚠️ 1 P2

⚠️ P2 medium — mobile/src/main/java/net/activitywatch/android/StagingReset.kt:15

The prefKey function concatenates the hostname directly into the SharedPreferences key string. Hostnames on Android are derived from device model and user-set device name, and can contain characters that are valid in SharedPreferences keys but may collide or be ambiguous. For example, if one hostname is 'a_b' and another is 'a' with a suffix '_b', the keys 'stagingResetDone_a_b' and 'stagingResetDone_a_b' would collide. More realistically, the hostname is sanitized by SanitizedHostnameMigration, but the raw device name could contain underscores. The collision would cause one hostname's reset flag to be shared with another, so if hostname A completes the reset, hostname B would skip it even though its staging db was never reset. This is a real correctness issue if two different hostnames can produce the same prefKey. The PR description says the flag is per hostname, but the key is not namespaced or length-prefixed, so collisions are possible. The impact is that a device that changes its hostname (e.g., user renames the device) could inherit a stale flag and skip the reset, leaving duplicate staging data. The severity is low because hostname changes are rare and the reset is a one-time cleanup, but it is a latent bug.

fun prefKey(hostname: String) = "stagingResetDone_" + hostname.length + "_" + hostname

How this was verified: Checked the hostname source: getDeviceName() returns deviceHostname(appContext), which is sanitized by SanitizedHostnameMigration. The sanitization replaces invalid characters, but underscores are allowed. Two different raw device names could sanitize to the same string, or a hostname could be a prefix of another with an underscore. The prefKey does not include a separator that cannot appear in the hostname, so collisions are possible.

Consensus: 2/3 passes agreed — pass 1 looked and disagreed
Distinct keys: 1 (general)

Files changed (4) — the diff as I read it
  • mobile/src/main/java/net/activitywatch/android/AWPreferences.kt — Adds isStagingResetDone/setStagingResetDone(hostname) backed by a per-hostname SharedPreferences key.
  • mobile/src/main/java/net/activitywatch/android/StagingReset.kt — New object with runOnce(isDone, markDone, reset) that parses the native JSON response and returns AlreadyDone/Done/Failed, catching Throwable.
  • mobile/src/main/java/net/activitywatch/android/SyncInterface.kt — Declares external resetStaging(hostname), adds resetStagingOnce(hostname) helper, and calls it in syncBothAsync before syncBoth.
  • mobile/src/test/java/net/activitywatch/android/StagingResetTest.kt — New unit tests for runOnce covering success, already-done, native failure, malformed JSON, UnsatisfiedLinkError, and per-hostname keys.
Previous review passes
commit score findings engine when
e0bcfec8d72e 3/5 1 llm 2026-10-07 09:32 UTC
8ee56bef48c1 3/5 2 llm 2026-10-14 11:26 UTC
8ee56bef48c1 4/5 4 llm 2026-10-14 12:28 UTC

Reviewed 4e0745965b8e · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 101s · about this reviewer

Maintainer commands

@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.

Comment thread mobile/src/main/java/net/activitywatch/android/StagingReset.kt Outdated
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

The Rust side is now up in ActivityWatch/aw-server-rust#815. It adds resetStaging(hostname) with the signature and JSON shapes this PR expects. It removes test.db/sync.db and their -wal/-shm sidecars for the own {hostname}/{device_id} dir only, and refuses any path that resolves outside the sync dir. Until a submodule bump carries it, this PR still sees UnsatisfiedLinkError and the flag stays unset, as described above.

Implement automatic one-shot reset of the phone's staging database to remove
pre-ActivityWatch/aw-server-rust#713 duplicates. The reset:
- Runs once per device (SharedPreferences flag)
- Executes before first push after upgrade
- Runs under syncInFlight guard (no concurrent access)
- Requires corresponding Rust JNI implementation in aw-sync/src/android.rs

Changes:
- Add JNI binding for resetStaging(hostname)
- Implement one-shot logic with device-specific preference tracking
- Integrate into sync pre-operations (push and full-sync paths)
- Add unit tests for one-shot behavior
- Document Rust-side implementation requirements

The reset removes staging files for own device only, preserving peer data.
Subsequent push rebuilds staging from clean local source, and desktop pull
will recognize re-imported records as duplicates (via #713 fingerprint fix).

Closes: ActivityWatch#296
Relates-to: ActivityWatch/aw-server-rust#717
Git-Session-Id: e1f2
…ive symbol

- Add AWPreferences.isStagingResetDone/setStagingResetDone (the previous
  getBoolean/setBoolean calls did not exist).
- Extract the one-shot flag logic into StagingReset.runOnce, free of Android
  types, and replace the Mockito tests (no Mockito dependency; the tests
  asserted nothing) with JVM tests of that logic.
- Catch LinkageError: until aw-server-rust ships resetStaging, the native call
  throws UnsatisfiedLinkError, which escaped both catch (Exception) blocks and
  would kill the sync executor before its callback cleared syncInFlight.
- Run the reset only inside the full-sync path, which is the one that holds
  syncInFlight (push/pull-all wrappers have no callers and no guard).
- Drop STAGING_RESET_IMPLEMENTATION.md from the repo root; the Rust spec lives
  in ActivityWatch#296.

Git-Session-Id: ac9fc99f-bc9b-569f-b59a-2817d7711006
@TimeToBuildBob
TimeToBuildBob force-pushed the fix/reset-staging-one-shot branch from e0bcfec to 8ee56be Compare October 14, 2026 11:13
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased onto master (resolved conflict in AWPreferences.kt: kept both hasMigratedNotifyOptIn/setNotifyOptInMigrated from master and isStagingResetDone/setStagingResetDone from this PR). CI running.

Comment thread mobile/src/main/java/net/activitywatch/android/SyncInterface.kt
Comment thread mobile/src/main/java/net/activitywatch/android/StagingReset.kt
Comment thread mobile/src/main/java/net/activitywatch/android/SyncInterface.kt
Comment thread mobile/src/main/java/net/activitywatch/android/StagingReset.kt Outdated
Comment thread mobile/src/main/java/net/activitywatch/android/AWPreferences.kt
Comment thread mobile/src/test/java/net/activitywatch/android/StagingResetTest.kt
…rows contract

The sync executor's performSyncAsync only catches Exception; any Error escaping
StagingReset.runOnce would leave syncInFlight permanently true, wedging all
future syncs as "already in flight". runOnce's doc says "Never throws", but the
previous catch(Exception)+catch(LinkageError) left non-LinkageError Errors
(e.g. AssertionError) able to escape.

Switch to catch(Throwable) to make the "Never throws" contract unconditional.
Add a test covering the non-LinkageError path.

Git-Session-Id: be171056-5acd-5fb9-aece-a62a90399bf0
Comment thread mobile/src/main/java/net/activitywatch/android/StagingReset.kt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant