# STOP-TB App: Push and Pull Sync Gaps and Mitigations

**Date:** 2026-10-28
**App source:** `~/Dev/PSMRI/STOP-TB-App`, branch `fix/safe-foreground-sync-workers` (PR #321 → `release-2.3`)
**Paths** below are relative to `app/src/main/java/org/piramalswasthya/stoptb/`.
**Related field evidence:** `stoptb-sync-log-anomalies-2026-08-07.md`, `stoptb-sync-log-anomalies-device3-2026-08-07-08.md`

---

## Summary

Users report sync problems. The causes are not one bug but four groups of design gaps:

1. **Data can be silently lost.** Pull can overwrite edits that are mid-push, push can mark an in-flight edit as synced, and nothing stops push and pull from running at the same time. In camp mode a full pull runs every ~10 seconds, so the overlap is constant.
2. **Failures are hidden or never retried.** Network errors become `Result.failure` instead of `retry`, a failed registration step cascades to TB and ABHA, one bad record blocks a whole batch every cycle, and several paths report success when they failed.
3. **Camp-hub connectivity depends on the app being open.** Background workers stop pushing after a single failed request and only resume when a user opens the app.
4. **Cost and security.** Every pull is a full download, and request and response bodies with PII are logged in release builds.

PR #321 already fixes saves made during a running push being dropped, expedites the registration push, and removes `setForeground()` from workers. Everything below is still open.

**How findings were checked.** Items marked ✅ were confirmed by reading the code directly. The others come from tracing the code with file and line references, and should be re-checked while fixing. Items marked 📊 also appear in the August field logs.

---

## A. Data loss and silent corruption (fix first)

### A1. Pull overwrites records that are being pushed ✅ (high)
- **What happens:** pull only protects local rows in `UNSYNCED` state (`repositories/BenRepo.kt:2287`). Push moves rows to `SYNCING` before uploading (`BenRepo.kt:562`). A pull that runs during a push replaces the local row with server data and marks it `SYNCED`. If that push then fails, `benSyncWithServerFailed` (`database/room/dao/BenDao.kt:1081`) re-queues a row that now holds the old server data, so the user's edit is gone.
- **Also:** the check happens while parsing, but the write happens later as a full-row `benDao.updateBen(serverBen)` (`BenRepo.kt:927-935`) with no re-check. An edit made between the two is overwritten.
- **Households have no guard at all.** They are upserted with `REPLACE` (`HouseholdDao.kt:15`). The only check is `processed == "P"` (`BenRepo.kt:2486`), and household save errors are swallowed (`BenRepo.kt:862-864`).
- **Mitigation:**
  - Skip any local beneficiary whose `syncState != SYNCED`, not just `UNSYNCED`.
  - Re-check and write inside one Room `withTransaction` in `upsertServerBeneficiariesSafely`.
  - Give households the same rule: skip if the household or any of its members is not synced. Consider adding a `syncState` column to households.

### A2. Push marks in-flight edits as synced ✅ (high)
- **What happens:** `benSyncedWithServer` runs `UPDATE BENEFICIARY SET processed='P', syncState=2 WHERE beneficiaryId IN (:ids)` (`BenDao.kt:1078`). If the user edits a beneficiary while its upload is in flight, the edit sets the row back to `UNSYNCED`, then this query marks it `SYNCED`. The edit is never sent.
- **Mitigation:** only mark rows that are still in flight: `... AND syncState = 1`. Apply the same pattern to the TB, household and vitals "mark synced" queries.

### A3. Push and pull run at the same time with no coordination ✅ (high)
- **What happens:**
  - Push (`PUSH-TO-AMRIT`) and pull (`PULL-FROM-AMRIT`) are independent unique chains in `work/WorkerUtils.kt`.
  - `processNewBenMutex` (`BenRepo.kt:58`) is only used by push. Pull never takes it.
  - In camp mode, `VolunteerActivity.startCampAutoPullLoop` triggers a pull every 10s (`ui/volunteer/VolunteerActivity.kt:603`, `WorkerUtils.campAutoPullIntervalMs`).
  - So push and pull overlap almost all the time, which turns A1 and A2 from rare races into the normal case.
- **Mitigation:**
  - `SyncGateWorker` treats a running pull chain as busy and re-arms.
  - `triggerAmritPullWorker` skips, without cancelling anything, while a push chain is running.
  - Reuse `getActiveUniqueWorkInfos` in `WorkerUtils.kt` for both checks.

### A4. Start-up recovery can race a running worker (low)
- **What happens:** `recoverOrphanedSyncStates()` runs on a background coroutine in `SakhiApplication.onCreate` (`SakhiApplication.kt:73-80`). When WorkManager starts the process to run a push, the reset can flip that push's rows from `SYNCING` back to `UNSYNCED` in the middle of the upload.
- **Mitigation:** run the reset before workers can start. One option is to run it inside the gate and push path behind the same mutex, instead of at application start.

---

## B. Failures that are hidden or never retried

### B1. Push never retries on network errors ✅ (high) 📊
- **What happens:** `postDataToAmritServer` catches every exception and returns `false` (`BenRepo.kt:657-672`). `PushToAmritWorker` turns that into `Result.failure`, not `retry` (`work/PushToAmritWorker.kt:30-36`). `BasePushWorker` only retries a `SocketTimeoutException` that escapes, and the repository never lets one escape. A flaky hub Wi-Fi therefore means no retry until the next save or app event.
- **Field evidence:** "Push worker drops the batch on 'No user logged in'", 6 times per device (Aug 7 report, item 5).
- **Mitigation:** return `Result.retry()` for transport failures (`IOException`, `UnknownHostException`, `ConnectException`) and keep `failure` for real server rejections. Have the repository return a result type that tells these apart, rather than a `Boolean`.

### B2. A failed registration step blocks TB and ABHA ✅ (high)
- **What happens:**
  - TB and ABHA push are chained after registration (`WorkerUtils.enqueuePushChain`).
  - When a prerequisite fails, WorkManager fails every dependent worker.
  - Counselling sync runs inside registration: `processNewBen` returns `uploadDone && counsellingSyncDone` (`BenRepo.kt:586-587`).
  - So a single counselling record that the server rejects stops all TB and ABHA data from syncing.
  - The Sync Dashboard even has a "Blocked by earlier failure" state for this (`SyncDashboardViewModel.kt:75-79`).
- **Mitigation:**
  - Move counselling sync out of `processNewBen` into its own worker in the chain.
  - Make TB and ABHA not depend on registration *succeeding*: TB records are already skipped while their beneficiary has no real ID, so ordering is enough. Run them as siblings after registration, or have registration report success when its own records were handled.

### B3. One rejected record blocks its whole batch, every cycle (high) 📊
- **What happens:**
  - All unsynced beneficiaries go in one request.
  - TB diagnostics, suspected and confirmed records go in chunks of 20 (`TBRepo.kt:1232, 1310, 1399`).
  - Nothing counts attempts per record or sets a record aside. A record the server always rejects fails its whole batch forever.
- **Field evidence:** device 3 on Aug 8. From 10:21, every registration attempt returned `statusCode=5000, Error in registration`. The backlog grew from 1 to 9 stuck beneficiaries, with 72 failed attempts in 28 minutes and no success afterwards.
- **Mitigation: isolate the failing record and keep retrying it. Never drop it.**
  The aim is to stop one record blocking others, not to give up on any record. Registration failures often have causes outside the record (server-side errors, like the Aug 8 `5000` responses), so every error is treated as retryable.
  1. **Isolate only when the server is clearly fine.**
     - If a whole batch fails, treat it as a server or network problem. No record is penalised; the queue is retried with normal backoff.
     - A record is isolated only when the rest of its batch goes through without it. To find it, retry in halves, then one record at a time.
     - Under this rule, the Aug 8 outage would have isolated nothing, and the whole backlog would have synced once the server recovered.
  2. **Isolated records keep retrying.**
     - They are sent one at a time after the main batch, with a backoff that grows to a cap: next cycle, 1 min, 5 min, 15 min, then hourly.
     - There is no maximum attempt count and no "failed for good" state.
  3. **Retry immediately when something changes.** Reset the backoff when:
     - any other record registers successfully (the server is healthy);
     - the hub reconnects;
     - the user logs in or the app is updated;
     - the user taps "Retry now" on the Sync Dashboard.
  4. **Keep the error history for diagnosis only.** Store `attempts`, `lastError`, `firstFailedAt` and `nextRetryAt` per record in a small key-value store in the existing encrypted shared preferences, keyed by entity and record ID, and delete the entry once the record syncs. No Room migration: `InAppDb` uses `fallbackToDestructiveMigration()`, so a bad migration would wipe unsynced data. This data controls backoff and visibility only; nothing is ever excluded from sync permanently.
  5. **Make stuck records visible.**
     - The Sync Dashboard (`SyncDao.kt`) lists isolated records with age, attempts and last error.
     - Any record unsynced for more than 24h is reported to Crashlytics as a non-fatal (IDs and error only, no PII).
     - The logout warning's unsynced count includes these records.
  6. **Apply the same rule to TB chunks of 20, vitals and counselling.**

### B4. TB and vitals push report success when records fail (medium)
- **What happens:**
  - `TBRepo.pushUnSyncedRecords()` and `VitalRepo.pushUnSyncedVitals()` always return `true` (`TBRepo.kt:1057-1058`, `VitalRepo.kt:88-89`).
  - Failed records stay `UNSYNCED`, but the worker succeeds, so it never retries and the dashboard shows no failure.
  - Vitals are also pushed twice per cycle (`PushToAmritWorker.kt:29` and `PushTBToAmritWorker.kt:30`).
  - The vitals 401 path calls itself with no depth limit (`VitalRepo.kt:218-238`).
- **Mitigation:**
  - Return accurate results.
  - Push vitals from one worker only.
  - Bound the 401 retry to one refresh-and-retry.

### B5. Pull reports failure as success ✅ (high)
- **What happens:**
  - If the first page fails, `getBeneficiariesFromServerForWorker` returns `-1`. The worker then calls `getBenForPage` with `numPages = -1`, the loop never runs, and the run counts as a success.
  - `UpdatePrefForPullCompleteWorker` then marks the full pull complete.
  - TB pull helpers catch exceptions and still return `true` (`work/PullTBFromAmritWorker.kt:136+`), and `saveTBConfirmedCacheFromResponse` has an empty `catch` (`TBRepo.kt:1033`).
  - A `SQLiteConstraintException` in `getBenForPage` silently ends that page stride and still returns `true` (`PullFromAmritWorker.kt:166-169`).
- **Mitigation:**
  - Treat `-1` as failure.
  - Make TB helpers propagate failures.
  - On a constraint error, log the offending record and continue with the next page instead of abandoning the stride.

### B6. Unbounded pull retry loop ✅ (high)
- **What happens:** `-2` (socket timeout, or a token refresh that deliberately throws `SocketTimeoutException("Refreshed Token!")`) is retried immediately and forever: `while (numPages == -2)` (`PullFromAmritWorker.kt:76-78`), and the per-page loop does not advance on `-2`. A hub that keeps timing out holds the worker in a tight loop.
- **Mitigation:** allow a few attempts with backoff, then return `Result.retry()` so WorkManager's exponential backoff takes over. Replace the "throw a timeout to signal a token refresh" trick with an explicit result.

### B7. Records tied to a temporary beneficiary ID never sync (high)
- **What happens:**
  - **Vitals:** these keep the `benRegId` captured when the form opened (`ui/.../VitalScreenViewModel.kt:51,162`). The ID cascade in `updateToFinalBenId` (`BenDao.kt:1059`) only updates `benId`, so a vital recorded against a temp-ID beneficiary is sent with `beneficiaryRegID=0` every cycle.
  - **Counselling:** responses (`t_form_response.beneficiaryId`) have no foreign key, and the ID swap only touches `all_visit_history`. They are sent with the negative temp ID.
  - Through B2 and B3, these can also block other data.
- **Mitigation:**
  - Skip these records until the beneficiary has a real ID.
  - Re-map vitals and counselling IDs in `updateToFinalBenId`.
  - Resolve `benRegId` at push time instead of form-open time.

---

## C. Camp-hub connectivity and pull cost

### C1. Hub connectivity only recovers while the app is open ✅ (high)
- **What happens:**
  - One failed request plus a failed health check sets `campHubConnected = false` (`network/interceptors/CampModeUrlInterceptor.kt:51-53`).
  - From then on, `WorkerUtils.canPush()` skips every push.
  - The flag is only set back to true by `VolunteerActivity.startCampHubReconnect` (`VolunteerActivity.kt:613-627`), the reconnect screen or sign-in. All of these need an activity in the foreground.
  - The preference listener that pushes after a reconnect is also only registered while an activity is resumed (`VolunteerActivity.kt:73-83`, `HomeActivity.kt:100-108`).
- **Field evidence:** the Aug 7 report measured up to 11h 55m per device in the disconnected state. The traffic fallback to production described there appears fixed in the current interceptor, but the flag-driven push block remains.
- **Mitigation:** when the flag is false, `canPush()` (or the gate) pings the hub health URL, reusing the existing `isCampHubReachable` logic, and restores the flag instead of skipping. Pushes and pulls then recover in the background.

### C2. Hub traffic may go over mobile data (needs a device check, medium)
- **What happens:** nothing binds requests to the Wi-Fi network (no `bindProcessToNetwork`, no per-network `socketFactory`). If the hub's Wi-Fi has no internet and mobile data is on, Android may make cellular the default network, and requests to the hub's LAN address then fail.
- **Mitigation:** test on a field device first: hub Wi-Fi with no internet, mobile data on, background push. If it fails, route camp-hub OkHttp calls through the Wi-Fi `Network`'s `socketFactory`, using the Wi-Fi network already tracked by the `NetworkCallback` in `VolunteerActivity`.

### C3. Every pull re-downloads everything (high) 📊
- **What happens:**
  - The beneficiary pull sends only `pageNo` and `villageID` (`BenRepo.kt:824-828`).
  - TB suspected uses the epoch as `fromDate` (`TBRepo.kt:803`), and screening sends only the village.
  - In camp mode this full download (all beneficiary pages plus 9 TB datasets) repeats almost continuously.
  - `setLastSyncedTimeStamp` is only ever set to its default (`VolunteerViewModel.kt:47`, `SignInViewModel.kt:197`). The first-sync page preference is never reset, so `DIRECT` pulls keep skipping the early pages.
- **Field evidence:** screening record `id=460` was downloaded 939 times in about 10 hours, and one device produced 97 MB of logs in a day (device 3 report).
- **Mitigation:**
  - Raise the AUTO_PULL interval (for example to 60 to 120 seconds), and pull right after a successful push instead.
  - Add incremental pull using a last-successful-pull timestamp; this needs backend support for `fromDate` / `lastModified` filters.
  - Reset `firstSyncLastSyncedPage` when a full pull completes.
  - Drop the duplicated `startPage` fetch and the extra page caused by `<=` (`PullFromAmritWorker.kt:140-153`).

### C4. Logged-out user stops push with no retry (medium) 📊
- **What happens:** `processNewBen` throws `IllegalStateException("No user logged in!!")` (`BenRepo.kt:535`), which becomes `Result.failure` in `BasePushWorker`. `TokenExpiryManager` can force a logout during a background run after repeated refresh failures.
- **Mitigation:** treat "no user" as `retry` (`BasePushWorker` already does this for the pre-check) and never mark records as failed because of it.

---

## D. Duplicates, auth and security

### D1. Retries can create duplicates on the server (high, backend decision)
- **What happens:**
  - After a timeout, `postDataToAmritServer` re-sends the whole batch up to 3 times (`BenRepo.kt:661`), even though the server may already have processed it.
  - `BenPost`, the TB request models, vitals and counselling carry no client-generated idempotency key. Only General OPD has a `submissionId`.
  - Beneficiary ID creation (`createBenIdAtServerByBeneficiarySending`) is not idempotent either, so a timeout after the server created an ID can produce a duplicate beneficiary.
  - The Aug 7 report saw screening records pushed twice; the server upserted them, which suggests some server-side de-duplication.
- **Mitigation:**
  - Agree with the Common-API / FLW-API owners on an idempotency key (a client UUID per record) and server-side upsert behaviour.
  - Until then, drop the in-repository timeout retry and let WorkManager retry the whole worker after backoff.

### D2. PII in release logs ✅ (high) 📊
> **Deferred (2026-10-28):** release HTTP body and payload logs are needed for field debugging for now. Not part of PR #321.

- **What happens:**
  - `HttpLoggingInterceptor` is set to `Level.BODY` in all builds; the `BuildConfig.DEBUG` check is commented out (`di/AppModule.kt:136-140`).
  - `SyncLogTree` is planted in release builds (`SakhiApplication.kt:57`). It writes sync-related lines to logcat (`SyncLogTree.kt:47-64`) and to the exportable sync log file.
  - Payload JSON with beneficiary and counselling data is logged explicitly (`BenRepo.kt:611`, counselling repository).
- **Field evidence:** `firstName`, `lastName` and `fatherName` appear 10,009 times each in one device's logs (device 3 report).
- **Mitigation:**
  - Set `HttpLoggingInterceptor.Level.NONE` in release, or `BASIC` behind a remote flag.
  - Remove payload JSON logging.
  - Limit `SyncLogTree` to status lines (counts, IDs, HTTP codes), never bodies.

### D3. Stored password (low)
- **What happens:** the plaintext password is kept inside the logged-in user object (`PreferenceDao.kt:87-93`) and in `UserCache` (`User.kt:19`). Both are in encrypted storage. `refreshTokenTmc` no longer uses the password (`UserRepo.kt:195-218`).
- **Mitigation:** stop storing the password if offline login doesn't need it; otherwise store only a salted hash.

---

## E. Housekeeping

- **E1. Dead push workers:** `CbacPushToAmritWorker`, `NCDReferPushtoAmritWorker`, `PushMalaria/Filaria/Leprosy/LeprosyFollowUp/Aes/KalaAzar` workers, `FormSyncWorker`, `NDCFollowUpPushWorker`, `PushToD2DWorker` and `triggerD2dSyncWorker` are never enqueued. Remove them, or wire them up if those programmes are in scope.
- **E2. No "last successful sync" time** in the UI. Add one per entity to the Sync Dashboard, so field staff and support can tell "pending" from "stuck".
- **E3. Sync Dashboard ID clash:** Diagnosis and TB Confirmed both use id 23 (`SyncDao.kt:86,91`).
- **E4. Stale comments** on foreground protection: fixed in PR #321.

---

## Delivery plan

Incremental pull (C3) is deferred until the API supports it. No step below needs a Room migration.

Each step is a separate PR to `release-2.3` after PR #321 merges. The order follows risk to data.

| # | PR | Gaps | Notes |
|---|---|---|---|
| 1 | Data integrity | A1, A2, A3, A4 | Smallest change with the biggest data-loss reduction. Add DAO unit tests: an edit made during `SYNCING` must survive both push completion and a pull. |
| 2 | Failure semantics | B1, B2, B4, B5, B6, C4 | Result type for repositories (success / retryable / rejected); move counselling to its own chain member; bound retries. |
| 3 | Failing-record isolation and temp IDs | B3, B7 | Per-record retry state in encrypted shared preferences (no Room migration). Isolated records keep retrying with capped backoff and are never dropped; show them on the Sync Dashboard and report records unsynced >24h to Crashlytics. |
| 4 | Camp connectivity and pull cost | C1, C2, C3 | Do the C2 device check first. Incremental pull depends on the backend. |
| 5 | Logging and security | D2, D3 | Can ship early and independently; low risk. |
| 6 | Cleanup | E1, E2, E3 | |
| backend | Idempotency | D1 | Raise with API owners; also chase the Aug 8 `statusCode=5000` registration failure. |

## How to verify

- **Build:** `sh gradlew :app:compileStoptbUatDebugKotlin -x processStoptbUatDebugGoogleServices`, plus unit tests with `sh gradlew :app:testStoptbUatDebugUnitTest`. Run `gradlew` through `sh` because it isn't executable in the repo, and skip the Google Services task because there is no local `google-services.json`.
- **Device tests in a camp setup:**
  1. Edit a beneficiary while a push is running: the edit reaches the server.
  2. Edit a beneficiary while a pull is running: the edit is not overwritten.
  3. Power off the hub mid-push: workers retry, and nothing is marked synced or permanently failed.
  4. Inject one rejected counselling record: TB and ABHA still sync, the record shows on the Sync Dashboard as retrying, and it syncs on its own once the server accepts it.
  5. Make the server reject all registrations, then restore it: no record is isolated, and the whole backlog syncs within one cycle of recovery (the Aug 8 scenario).
  6. Background the app, drop and restore hub Wi-Fi: push resumes without opening the app.
  7. Hub Wi-Fi with no internet and mobile data on: background push reaches the hub (C2).
  8. Release build: `adb logcat` and the exported sync log contain no request or response bodies or names.
- **Field check:** compare a day of sync logs before and after against the August baseline (pull volume, fallback time, stuck registrations).