pneumo

M4: Connection service and sync engine (ENG-259) #24

Merged

demi.dev 1w ago

targeting main from stoopkidddd/eng-259-m4-connection-service-and-sync-engine-foreground-websocket

Merged opened by and assigned to

demi.dev 1w ago targeting main from stoopkidddd/eng-259-m4-connection-service-and-sync-engine-foreground-websocket

Linear: https://linear.app/engineereddev/issue/ENG-259

What's in this PR (54 files, all under apps/android/):

service/: ConnectionService, a remoteMessaging foreground service with a silent connection channel ("Connected to " / "Reconnecting…" / "Offline", tap opens the app), START_STICKY. Started after pairing, on launch when paired and by BootReceiver (BOOT_COMPLETED, MY_PACKAGE_REPLACED); stopped only on sign-out or revoke.
net/ws/: WsClient (OkHttp, pingInterval 0): hello with since = lastPushId, pong, a 2 × WS_PING_SECONDS + 15 s watchdog, full-jitter backoff 1/2/5/10/30/60 s reset after 60 s, an immediate reconnect from the NetworkCallback, 4401 → sign-out flow, 1012 → retry after 1–3 s; StateFlow<ConnectionState>.
data/db/: Room v1 (pushes with envelope JSON, file id/size and a notified flag; devices; sync_state), with the exported schema. Plaintext is never stored.
sync/: SyncEngine applies frames, pages GET /api/pushes on resync until it overlaps Room, advances lastPushId per push, and hands each eligible push to PushSink once (a no-op for now).
Settings shows the live connection state.
Roborazzi: 6 new baselines connection-{connected,reconnecting,offline}-{light,dark} and 6 re-recorded on purpose main-shell-{settings,settings-busy,sign-out-error}-{light,dark}. The commit says so.

Verify (lead, rebased on e5f90f2):

mise run check:android → exit 0 (Spotless, detekt, Lint clean, Roborazzi verify)
./gradlew pixel6Api34DebugAndroidTest --rerun → BUILD SUCCESSFUL, "Running tests on devices: pixel6Api34"
./gradlew testDebugUnitTest --rerun → 146 tests, 0 failures
Implementer's forced-Doze acceptance (local server, emulator): connected for all 10 one-minute samples with screen off and deviceidle force-idle; after unforce, Room held all 12 test pushes (10 sent during Doze), all marked notified. Package replacement restored the connection.

Deviations:

No managed-device harness server exists yet (ENG-268), so connection, Doze and package replacement were checked with the issue's permitted local-server fallback.
Android 14 raised the active foreground-service channel from MIN to LOW; the original importance stays MIN and sound stays off.
Exactly-once PushSink hand-off is durable-claim-based: process death between claim and sink can drop one notification but never duplicates it. Proposed spec clarification: idempotent notification delivery.

🤖 Generated with Claude Code

Commits 0

Could not load commits.

Comments

v0 1w ago

demi.dev 1w ago

pneumo-reviewer: round 1, REQUEST_CHANGES #

The service, WebSocket client, Room v1 and the status UI follow §9 and §14.4 closely: backoff, watchdog, 4401, 1012, pong, the notified claim and secret hygiene are correct and tested; check:android and the JVM tests pass. The implementer's three deviations are acceptable. Two catch-up gaps in SyncEngine block the merge.

FIX (score ≥ 5)

often=2 bad=6 score=12 — sync/SyncEngine.kt:61-63: with a null cursor, a gap is never caught up. After pairing, lastPushId stays null until the first live push; the server answers since: null with no replay, and the engine treats ready/replay_done as no-ops. A push sent while the phone is offline before its first received push is lost silently. Fix: when hello went out with since = null, run the resync catch-up on replay_done, bounded below by the pairing point (e.g. seed sync_state at claim time). Test: null cursor, disconnect, push, reconnect.
often=2 bad=5 score=10 — sync/SyncEngine.kt:80-108: resync stops only when a listed id is already in Room. If the cursor push was deleted and Room is empty, it pages the whole server history and hands every old push to PushSink (a notification flood once the notifier lands). Fix: also stop at the first push not after the stored (lastCreatedAt, lastPushId) position. Test: cursor push deleted, Room empty, resync.

FILE (score < 5, code left alone, recorded on ENG-275)

score=4 — no exception handler on the service/application scopes; non-IO exceptions can crash-loop under START_STICKY.
score=3 — cursor never moves after a resync overlap with nothing pending; each reconnect resyncs again.
score=4 — startForeground() on every state change; use NotificationManager.notify (check in the M5 soak).
score=4 — watchdog/backoff on Main-dispatcher uptime delays pause during CPU suspend (check in the M5 soak).
score=4 — WS loop and replay JSON decoding run on Dispatchers.Main.immediate.
score=3 — when the notifier lands, call the sink before claiming (tag = push id makes a re-post idempotent).

Spec §14.4, §14.2, §14.9 OK; §9/§14.5 OK apart from findings 1–2. DoD present (39 JVM tests, 6 new and 6 re-recorded goldens, local-server Doze check). Next: omp round 2 for findings 1–2.

v1 1w ago

demi.dev 1w ago

pneumo-reviewer: round 2 (head 31177a6), APPROVE #

Both round-1 FIX findings are resolved. Both catch-up paths share one lower bound: the stored (lastCreatedAt, lastPushId) position, seeded at pairing with this device's server createdAt and an empty id. Room now only gets post-pairing pushes on the null-cursor path and on the deleted-cursor resync path. The android tree of the rebased a3945e1 is identical to round 1; no baseline, schema or packages/ changes.

FIX 1 resolved: ready with a null cursor runs the bounded catch-up (SyncEngine.kt:62). Test nullCursorReconnectCatchesUpFromPairingPosition fails without the fix.
FIX 2 resolved: paging stops at the Room overlap or the first push not after currentPosition(). Test deletedCursorResyncStopsAtStoredPositionWithEmptyRoom fails without the bound.
Deviations accepted: catch-up on ready (equivalent to replay_done for since: null); seed from /api/me device.createdAt with an empty id.

New FILE findings (score < 5, recorded on ENG-275):

score=4 — no test covers the pairing seed (PairingRepository.kt:124, AppContainer.kt:54-56); removing it would reintroduce unbounded catch-up.
score=4 — the ready check reads the cursor at processing time, not the since actually sent; a rare socket-only drop during the first catch-up can skip it.

Verify: lead check:android exit 0, testDebugUnitTest --rerun 148/0 failures, pixel6Api34DebugAndroidTest --rerun 8/0; reviewer reran check and unit tests. CI 3/3 on 31177a6.

Versions

pneumo-reviewer: round 1, REQUEST_CHANGES #

pneumo-reviewer: round 2 (head 31177a6), APPROVE #