fix(send): drainOutbox re-sends rows parked as 'may have sent' — duplicate emails #489

Open
opened 2026-07-10 19:14:42 +00:00 by JMR-dev · 0 comments
JMR-dev commented 2026-07-10 19:14:42 +00:00 (Migrated from github.com)

Verified finding(s) from the 2026-07-09 whole-repo multi-agent review (independent finder, then adversarial verifier; verdict CONFIRMED).

Triage: above the cut — fix dispatched immediately; this issue tracks the fix to Done.

app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt:67 — high

A Graph send marked 'may have sent' (mayHaveSent=true) is left in the outbox but drainOutbox() re-sends every row returned by outboxDao.getAll(), so any later drain automatically re-sends it, duplicating the email — violating the code's own no-duplicate invariant ('leave it queued and let the user decide').

Failure scenario: Outlook user sends message A; Graph accepts it but the response is lost (GraphHttp readStatus throws mayHaveSent=true), so sendQueued sets lastError and returns failed=false. Minutes later the user queues message B (MailRepositoryImpl.sendScheduler.sendNow -> new drain), or another outbox row fails transiently in the same drain (anyFailed -> Result.retry, WorkManager re-runs in ~30s). Either way drainOutbox iterates outboxDao.getAll() — which has no status filter (OutboxDao.kt:15 'SELECT * FROM outbox') — and sendQueued sends A again via sendOutlook, delivering A twice to all recipients with no user decision involved.

Verifier justification (CONFIRMED): SendWorker.kt:138-145 deliberately leaves a GraphSendException(mayHaveSent=true) row queued ("let the user decide") and returns failed=false so WorkManager won't retry — but the hold is not persisted anywhere. OutboxEntity (app/src/main/kotlin/org/libremail/data/local/entity/OutboxEntity.kt:19) has only lastError: String?, no status column; OutboxDao.getAll() (OutboxDao.kt:15) is SELECT * FROM outbox ORDER BY createdAt with no filter; and drainOutbox() (SendWorker.kt:67-75) unconditionally calls sendQueued() on every row. Trigger: Outlook user's message A hits the mayHaveSent case; then the user sends message B — MailRepositoryImpl enqueues via SendScheduler.sendNow() (SendScheduler.kt:20-31, ExistingWorkPolicy.REPLACE), a fresh drain runs, and A is sent again via sendOutlook with no user decision. The same happens if any other row in the original drain fails transiently (anyFailed → Result.retry re-runs the worker in ~30s). If Graph did deliver A the first time, recipients get it twice — exactly the duplication the code's own comment (line 140) claims to prevent. No guard exists elsewhere: nothing filters on lastError, and there is no UI-only resume gate.

Defective line: val pending = outboxDao.getAll() // SendWorker.kt:67, feeding: for (entity in pending) { sendQueued(...) } — while lines 139-141 state: "Graph may already have delivered this; auto-retrying (or any other send) would duplicate it, so leave it queued with a clear status and let the user decide."

Fix hint: Persist the hold: add a status/held column (or a heldForUserDecision flag) to OutboxEntity, set it in the mayHaveSent branch instead of just setError, and have drainOutbox query only non-held rows (new DAO query); the user's explicit "retry" action clears the flag before calling sendNow(). Requires a Room schema bump + migration test.

app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt:72 — high

drainOutbox iterates outboxDao.getAll() with no exclusion for rows parked with the GraphSendException.mayHaveSent 'Send status unknown' error, so the very duplication the mayHaveSent guard documents ('auto-retrying or any other send would duplicate it... let the user decide') happens automatically on the next drain.

Failure scenario: A Graph send throws with mayHaveSent=true (e.g. timeout after the request was accepted); the row is left queued with setError and not counted as failed. The user then sends any new message — ComposeViewModel.send → MailRepositoryImpl.sendMessage → sendScheduler.sendNow() → SendWorker drains ALL rows including the ambiguous one and calls graphSender.send on it again — the recipient receives the email twice. The same auto re-send happens when a different queued message fails (anyFailed → Result.retry re-runs the whole drain on backoff).

Verifier justification (CONFIRMED): The mechanism is fully verified in the code. When a Graph send throws with mayHaveSent=true, SendWorker.sendQueued only calls outboxDao.setError(entity.id, "Send status unknown — check your Sent folder, then retry or cancel") and returns failed=false — the row stays in the outbox table (OutboxEntity has only a lastError: String? column, no held/parked state flag; see app/src/main/kotlin/org/libremail/data/local/entity/OutboxEntity.kt line 19). OutboxDao.getAll() is SELECT * FROM outbox ORDER BY createdAt with no exclusion (OutboxDao.kt line 15), and drainOutbox at SendWorker.kt line 72 iterates every row and calls sendQueued unconditionally — sendQueued never inspects entity.lastError. Concrete trigger: (1) GraphHttp.kt line 110 throws GraphTransportException("Graph request sent but no response received", mayHaveSent = true) after a request was transmitted but the response was lost; the row is parked. (2) The user sends any other message: MailRepositoryImpl.sendMessage (line 526) calls sendScheduler.sendNow(), which enqueues SendWorker; drainOutbox picks up the parked row and graphSender.send is invoked on it again — if Graph had accepted the first request, the recipient gets the email twice. The same happens when a different queued row fails (anyFailed → Result.retry re-drains everything on backoff). The unit test at SendWorkerTest.kt line 283 only verifies the single-run behavior (Result.success, no fallback); nothing tests or prevents the next drain re-sending the parked row, so the guard's own documented intent ("auto-retrying (or any other send) would duplicate it... let the user decide") is defeated. This is not one of the known-intentional behaviors.

Defective line: for (entity in pending) { val failed = sendQueued(entity, outboxDao, accountDao, connectionFactory, attachmentUriGrants)

Fix hint: Add a held/needs-user-decision flag to OutboxEntity (schema migration) that setError sets in the mayHaveSent branch; exclude held rows from the drain query (or skip them in sendQueued) and clear the flag only on an explicit per-row user retry (retryOutbox currently just re-drains everything via sendNow, so it needs a row-targeted reset).

Verified finding(s) from the 2026-07-09 whole-repo multi-agent review (independent finder, then adversarial verifier; verdict **CONFIRMED**). **Triage: above the cut — fix dispatched immediately; this issue tracks the fix to Done.** ## `app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt:67` — high A Graph send marked 'may have sent' (mayHaveSent=true) is left in the outbox but drainOutbox() re-sends every row returned by outboxDao.getAll(), so any later drain automatically re-sends it, duplicating the email — violating the code's own no-duplicate invariant ('leave it queued and let the user decide'). **Failure scenario:** Outlook user sends message A; Graph accepts it but the response is lost (GraphHttp readStatus throws mayHaveSent=true), so sendQueued sets lastError and returns failed=false. Minutes later the user queues message B (MailRepositoryImpl.sendScheduler.sendNow -> new drain), or another outbox row fails transiently in the same drain (anyFailed -> Result.retry, WorkManager re-runs in ~30s). Either way drainOutbox iterates outboxDao.getAll() — which has no status filter (OutboxDao.kt:15 'SELECT * FROM outbox') — and sendQueued sends A again via sendOutlook, delivering A twice to all recipients with no user decision involved. **Verifier justification (CONFIRMED):** SendWorker.kt:138-145 deliberately leaves a GraphSendException(mayHaveSent=true) row queued ("let the user decide") and returns failed=false so WorkManager won't retry — but the hold is not persisted anywhere. OutboxEntity (app/src/main/kotlin/org/libremail/data/local/entity/OutboxEntity.kt:19) has only `lastError: String?`, no status column; OutboxDao.getAll() (OutboxDao.kt:15) is `SELECT * FROM outbox ORDER BY createdAt` with no filter; and drainOutbox() (SendWorker.kt:67-75) unconditionally calls sendQueued() on every row. Trigger: Outlook user's message A hits the mayHaveSent case; then the user sends message B — MailRepositoryImpl enqueues via SendScheduler.sendNow() (SendScheduler.kt:20-31, ExistingWorkPolicy.REPLACE), a fresh drain runs, and A is sent again via sendOutlook with no user decision. The same happens if any other row in the original drain fails transiently (anyFailed → Result.retry re-runs the worker in ~30s). If Graph did deliver A the first time, recipients get it twice — exactly the duplication the code's own comment (line 140) claims to prevent. No guard exists elsewhere: nothing filters on lastError, and there is no UI-only resume gate. **Defective line:** `val pending = outboxDao.getAll() // SendWorker.kt:67, feeding: for (entity in pending) { sendQueued(...) } — while lines 139-141 state: "Graph may already have delivered this; auto-retrying (or any other send) would duplicate it, so leave it queued with a clear status and let the user decide."` **Fix hint:** Persist the hold: add a status/held column (or a `heldForUserDecision` flag) to OutboxEntity, set it in the mayHaveSent branch instead of just setError, and have drainOutbox query only non-held rows (new DAO query); the user's explicit "retry" action clears the flag before calling sendNow(). Requires a Room schema bump + migration test. ## `app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt:72` — high drainOutbox iterates outboxDao.getAll() with no exclusion for rows parked with the GraphSendException.mayHaveSent 'Send status unknown' error, so the very duplication the mayHaveSent guard documents ('auto-retrying or any other send would duplicate it... let the user decide') happens automatically on the next drain. **Failure scenario:** A Graph send throws with mayHaveSent=true (e.g. timeout after the request was accepted); the row is left queued with setError and not counted as failed. The user then sends any new message — ComposeViewModel.send → MailRepositoryImpl.sendMessage → sendScheduler.sendNow() → SendWorker drains ALL rows including the ambiguous one and calls graphSender.send on it again — the recipient receives the email twice. The same auto re-send happens when a different queued message fails (anyFailed → Result.retry re-runs the whole drain on backoff). **Verifier justification (CONFIRMED):** The mechanism is fully verified in the code. When a Graph send throws with mayHaveSent=true, SendWorker.sendQueued only calls `outboxDao.setError(entity.id, "Send status unknown — check your Sent folder, then retry or cancel")` and returns failed=false — the row stays in the outbox table (OutboxEntity has only a `lastError: String?` column, no held/parked state flag; see app/src/main/kotlin/org/libremail/data/local/entity/OutboxEntity.kt line 19). OutboxDao.getAll() is `SELECT * FROM outbox ORDER BY createdAt` with no exclusion (OutboxDao.kt line 15), and drainOutbox at SendWorker.kt line 72 iterates every row and calls sendQueued unconditionally — sendQueued never inspects entity.lastError. Concrete trigger: (1) GraphHttp.kt line 110 throws GraphTransportException("Graph request sent but no response received", mayHaveSent = true) after a request was transmitted but the response was lost; the row is parked. (2) The user sends any other message: MailRepositoryImpl.sendMessage (line 526) calls sendScheduler.sendNow(), which enqueues SendWorker; drainOutbox picks up the parked row and graphSender.send is invoked on it again — if Graph had accepted the first request, the recipient gets the email twice. The same happens when a different queued row fails (anyFailed → Result.retry re-drains everything on backoff). The unit test at SendWorkerTest.kt line 283 only verifies the single-run behavior (Result.success, no fallback); nothing tests or prevents the next drain re-sending the parked row, so the guard's own documented intent ("auto-retrying (or any other send) would duplicate it... let the user decide") is defeated. This is not one of the known-intentional behaviors. **Defective line:** `for (entity in pending) { val failed = sendQueued(entity, outboxDao, accountDao, connectionFactory, attachmentUriGrants)` **Fix hint:** Add a held/needs-user-decision flag to OutboxEntity (schema migration) that setError sets in the mayHaveSent branch; exclude held rows from the drain query (or skip them in sendQueued) and clear the flag only on an explicit per-row user retry (retryOutbox currently just re-drains everything via sendNow, so it needs a row-targeted reset).
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMail#489