Migrate wear-sync onto the shared DurableEventQueue - #296
Open
ScottMorris wants to merge 4 commits into
Open
Conversation
ScottMorris
changed the title
ScottMorris
added
architecture
3 tasks
ScottMorris
marked this pull request as ready for review
August 14, 2026 04:44
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fe149f10d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rableEventQueue Replaces the plugin's bespoke `WearSyncQueue` (a hand-rolled SharedPreferences JSON array) with `WearSyncEventQueue`, a thin wrapper around native-bus's shared `DurableEventQueue` log. Each Play Services message path (e.g. `/threshold/save_alarm`) becomes the queue's topic; the raw message string is its opaque payload. This is Phase 2B of issue #255 -- a behaviour-preserving refactor, so nothing about what data flows to/from Rust or the watch changes, only how pending messages are stored/drained internally on the Kotlin side. **Why:** `WearSyncQueue` was the design template `DurableEventQueue` was modelled on in Phase 1 (see native-bus's own KDoc), so consolidating onto the shared implementation removes a duplicate, independently-maintained queue format now that a reusable one exists. **What changed:** - `WearSyncPlugin.onWatchMessage` now always enqueues first, then drains immediately if the pipeline is ready -- "immediate dispatch" is just "enqueue, then drain right away" rather than a separate code path, so there is exactly one way a message ever reaches Rust (through `drainQueuedMessages`), matching the Unified design's decision 7. - `WearSyncEventQueue` migrates any leftover `WearSyncQueue`-format entries (the old `pending_messages` SharedPreferences key) into the new `DurableEventQueue` log the first time either `enqueue` or `drainAll` runs, preserving each entry's original `timestamp` as `publishedAt`, then removes the old key. This is one-way -- documented in the plugin's README, since `RELEASE_NOTES.md` is only written at actual release-tagging time, not per-phase. - Added the real Gradle project dependency (`implementation(project(":tauri-plugin-native-bus"))`) and the matching CI settings-synthesis line for wear-sync's `test-kotlin-plugins` job, both deliberately deferred from Phase 1. - Deleted `WearSyncQueue.kt` -- its logic has no remaining callers. **Verification:** - `./gradlew -p plugins/wear-sync/android testDebugUnitTest` and the equivalent native-bus task both green in the CI mobile container, matching `.github/workflows/test.yml`'s exact settings-synthesis pattern. - `cargo check --workspace` clean -- confirms the Rust side is untouched. - A full `pnpm tauri android build --debug -t aarch64 --ci` succeeded end to end, proving the new Gradle dependency resolves in the real app build, not just the isolated per-plugin CI job.
…round-trip Plain JUnit 4 tests against an in-memory `KeyValueStore` fake, following the same convention native-bus's own test suite established (no Robolectric, `InMemoryKeyValueStore` duplicated here rather than shared across module test source sets, mirroring `NativeEventLog`'s existing duplication-over-sharing rationale in this plugin). `DurableEventQueue`'s own drain/commit/corrupt-entry behaviour is already covered by native-bus's suite, so these focus on what's new here: the legacy `WearSyncQueue`-format migration (topic/payload/publishedAt mapping, the old key being removed, malformed/corrupt entries not blocking the rest), and the enqueue/drain round-trip that replaces `WearSyncPlugin.onWatchMessage`'s old direct `WearSyncQueue` calls. Note: `WearSyncPlugin.onWatchMessage`/`drainQueuedMessages` themselves aren't exercised directly, since this codebase's plain-JUnit Kotlin setup has no mocking framework or Robolectric to stand in for the `Activity`/`Channel` those methods need -- `WearSyncEventQueue` is the actual unit that carries the enqueue/drain/migration logic they delegate to.
…and restored field logs Addresses code review findings on the DurableEventQueue migration. **Data loss on partial delivery failure:** `WearSyncEventQueue.drainAll()` removed every envelope from the log before the caller had actually delivered any of them, defeating `DurableEventQueue`'s peek-then-commit design. If `channel.send()` threw partway through a batch in `WearSyncPlugin.drainQueuedMessages`, every remaining queued message was already gone -- silently lost, not retried. Replaced `drainAll()` with `peekAll()` (read-only) and `commit(eventIds)`; `drainQueuedMessages` now delivers each entry first and commits only the ones actually handed to the Channel, via a try/finally so a mid-batch failure still commits whatever succeeded before it and leaves the rest queued for the next drain. **Cross-instance race:** `WearMessageService.handleOfflineWrite` constructed a brand-new `WearSyncEventQueue` (and thus `DurableEventQueue`) on every call. `@Synchronized` locks each instance's own monitor, so two independently- constructed instances over the same SharedPreferences file gave zero mutual exclusion between `WearSyncPlugin`'s and `WearMessageService`'s enqueues -- two watch messages arriving close together could race and silently clobber each other. Added `WearSyncEventQueue.getInstance(context)`, a process-wide singleton accessor (mirroring how this plugin already holds other shared state via `WearSyncPlugin.instance`); both call sites now go through it. **Observability regression:** the old `WearSyncQueue` called `NativeEventLog` (which feeds the user-exported log via `requestWatchLogs`, no ADB needed) on every enqueue/drain/migration; the replacement only logged to logcat. Restored the equivalent `NativeEventLog` calls, gated on an optional `Context` so tests can still construct this class against an in-memory `KeyValueStore` with no Android framework available. **CI:** the settings-synthesis fix for wear-sync's native-bus dependency re-introduced the "unqualified `testDebugUnitTest` sweeps every included subproject" problem Phase 1 deliberately avoided. Fixed properly this time: both wear-sync's and native-bus's own `gradlew` invocations now use the qualified `:testDebugUnitTest` task path (leading colon = root project's own task only), so including native-bus in wear-sync's settings is genuinely harmless -- no redundant sweep, no cross-plugin failure misattribution. alarm-manager's invocation is intentionally left untouched (separate branch). **Verification:** - `./gradlew -p plugins/wear-sync/android :testDebugUnitTest` -- 13/13 pass (grew from 10: added peek-without-commit and partial-commit coverage). - `./gradlew -p plugins/native-bus/android :testDebugUnitTest` -- 16/16 pass, and confirmed via task output that it no longer runs as a side effect of wear-sync's own build (38 tasks executed vs. 50 before the qualified-path fix, with no `tauri-plugin-native-bus:testDebugUnitTest` task in the list). - `cargo check --workspace` clean.
…d Markdown, per code review Two real bugs from automated review on PR #296's WearSyncEventQueue, plus reflowing hard-wrapped Markdown paragraphs this branch had introduced. **Unsynchronized legacy migration.** WearMessageService and WearSyncPlugin both go through the same singleton WearSyncEventQueue instance, but migrateLegacyEntriesIfNeeded's check-then-act (read the legacy key, decide whether to migrate, write back) ran outside any lock -- DurableEventQueue's own `@Synchronized` methods only guard its own delegated calls, not this wrapper-level sequence, which reads/writes the same underlying key directly. Two concurrent callers could each pass the "has legacy entries" check before either wrote back, and the later migration write would silently clobber whatever the other caller had just enqueued. Fixed with a `lock` field shared across `enqueue`, `peekAll`, and `commit`, held for the full migration-decision-plus-delegated-operation sequence. **Non-atomic legacy migration write.** The migration wrote the new log and removed the legacy key as two separate SharedPreferences calls; a process death between them left both copies on disk, and the next access re-migrated the same entries again with fresh event IDs, delivering the same watch action twice. Fixed by routing that write through `KeyValueStore.batch()` (already used by alarm-manager's identical fix for the same class of bug), so the new log and legacy-key removal land as one atomic transaction. **Markdown formatting.** Reflowed every hard-wrapped prose paragraph this branch had introduced (in README.md, WearSyncEventQueue.kt, WearSyncPlugin.kt, WearMessageService.kt, build.gradle.kts, and the test files) into single unwrapped source lines per AGENTS.md's Markdown convention. **Tests:** WearSyncEventQueueTest grows to 15 cases (from 13), adding a new RecordingKeyValueStore fake (mirroring AlarmManagerPluginTest's) to assert migration goes through exactly one atomic batch() call, and a many-threads-racing-past-a-start-gate test proving no event is lost when concurrent enqueue calls race a pending legacy migration. Verified: full plugins/wear-sync/android + plugins/native-bus/android JUnit suites (15 + 12 tests, all passing) via `gradlew testDebugUnitTest` against the tauri-ci-mobile CI image. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013WVm6JTTpiSY8KSKJ3p5Qq
ScottMorris
force-pushed
the
feat/255-p2b-wear-sync-queue
branch
from
August 18, 2026 16:56
6fe149f to
6f605ec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Phase 2B of #255's implementation plan: migrates wear-sync's
WearSyncQueue(which the newDurableEventQueuein #294 was actually modelled on) onto the shared substrate too, so both plugins that need "queue events until Rust is up" logic share one implementation. Behaviour-preserving — nothing about what's sent/received to the watch or Rust changes, only the internal persistence mechanism.What's here
WearSyncEventQueuewrapsDurableEventQueue(path → topic, data → payload, timestamp → publishedAt);WearSyncQueue.ktdeleted, no remaining callers.pending_messageskey into the new log.onWatchMessagenow always enqueues then drains-if-ready (single code path, matching Design: shared native event-queue layer for plugin↔Rust handoff, plus cross-plugin native fan-out #255's "the log is the only delivery path" decision) instead of the old ready/queued split.WearSyncPlugin,WearMessageService) share one queue instance — the earlier per-call-construction pattern would have made the queue's internal@Synchronizedlocking meaningless (different instances, different monitors, no real mutual exclusion).NativeEventLogon enqueue/commit/migration, matching what the oldWearSyncQueuesurfaced through the user-exported log..github/workflows/test.yml: both alarm-manager's and wear-sync's synthesized CI settings now include the native-bus project, and every affectedgradlewinvocation uses a qualified:testDebugUnitTesttask path — this was the actual fix for the Gradle-sweep risk flagged during review (better than the "defer the include" workaround Add the shared native event bus plugin (native-bus) #294 shipped with), so native-bus's tests no longer run redundantly inside other plugins' CI steps.Testing checklist (automated, no device needed)
cargo check --workspacecleanpnpm tauri android buildsucceeds end-to-end; confirmed the qualified-task-path fix actually eliminates the sweep (38 Gradle tasks run for wear-sync's build vs. 50 before, with notauri-plugin-native-bus:testDebugUnitTestin the list)Review status
Reviewed via
/code-review hightwice. First pass found a real data-loss bug (the originaldrainAll()removed entries from the log before delivery was confirmed, so a mid-batch failure silently dropped the rest) and a real concurrency bug (a fresh queue instance per call defeated its own locking). Both fixed and re-verified with new test coverage. A flagged performance note (always persisting to SharedPreferences even on the already-ready fast path) was confirmed as an intentional tradeoff from #255's design, not a regression — left as-is.Stacked on #295, which is stacked on #294. Part of #255.