fix(chat): report stream liveness honestly and stop losing read pointers - #1365
Merged
Conversation
…inters Two defects behind the reported late messages and the unread badge that survives opening a chat. `openBidirectionalStream` retries internally, but `isStreamActive` is only ever set by `activateStream()` on a first response and cleared by an explicit teardown, so it stayed `true` for the whole retry window. Every external recovery trigger reads that flag or the reference's mere existence to decide whether to step in: the 30s heartbeat in `EventStreamDelegate` checks `isStreamActive`, and `open()` from lifecycle resume and network reconnect checks `isConnected`. With a stale `true`, all of them no-op while nothing is delivering, so recovery waited on the loop's own backoff — up to eight attempts and ~91s of sleeps before it gives up and the next heartbeat tick can reopen. The loop now marks the reference down at the top of each attempt, which is what the heartbeat needs to see. `ChatMemberDao.upsert` was a whole-row REPLACE, so a feed sync overwrote `pointers_json` with the server's copy. The READ pointer is advanced locally as messages come into view and reported afterwards, so any sync racing that report — or following a failed one — rewound the pointer and the chat re-reported as unread. Its sibling `ChatMetadataDao.upsert` already avoids this for `latest_event_sequence` and `analytics_counted_through`; `pointers_json` is the same kind of client-owned watermark and now merges by taking the greater value per pointer. The `FullRefresh` branch of the event stream wiped the member rows before re-inserting them, which defeated any merge, so it prunes departed members instead of clearing the table.
`ChatMemberDao.updatePointers` was a bare `UPDATE ... WHERE chat_id_hex = ? AND user_id_hex = ?`, which matches nothing when the member row is absent, so the pointer was dropped without a trace. Both callers can reach it in that state: a read is written the moment a message is on screen, which can beat the feed sync that writes the membership, and the event stream applies pointer updates for members the feed has not written yet. It is now `advancePointer`, which inserts the row when it is missing and otherwise carries the member's other pointers over. The read-merge-write moved into the DAO with it, so a feed sync landing between the read and the write can no longer be lost.
`advancePointer` was last-write-wins per pointer type, which left the stream path able to do what the feed sync no longer can. `EventStreamDelegate` applies `pointerUpdates` as they arrive, and one of those is the server's copy of the member's own pointer, which can sit behind a local advance that has not been reported yet — applying it put the chat back to unread. It now keeps whichever value is further ahead, the same rule `mergePointers` uses on a feed sync, so both writers agree.
Advancing the read pointer writes locally and then reports to the server, and the report is fire-and-forget: ChatViewModel drops the Result and nothing retries. The local pointer survives a failed report — the member row keeps whichever value is further ahead — so the two copies can stay apart indefinitely. This device's badge clears while the server still considers the chat unread, which keeps sending pushes for messages already read and shows them unread on every other device and after a reinstall. A feed payload carries the server's own copy of the pointer, so the sync is where the two can be compared. When the stored pointer is ahead of the fetched one, the sync emits ReadPointerUnreported and the coordinator re-sends the advance. Rather than an in-place retry, this rides the existing sync triggers — login, foreground, network reconnect, heartbeat — so a report lost to a process death is still recovered. The re-report deliberately skips advanceReadPointer: the local write and the message-received analytics already happened when the message was seen.
A stream that fails outside the reconnect loop's retryable set — INTERNAL, UNKNOWN, a ping timeout, or the loop running out of attempts — is not retried by the loop at all. Nothing else was watching for that: the heartbeat polls liveness every 30s, so the connection stayed down for up to a full interval after it died, and every message in that window arrived late. EventStreamingController now emits on streamFailures when it clears the ref for a stream that ended this way, and the heartbeat waits on that signal or the tick, whichever comes first. A pending signal is dropped when a stream is opened, so it cannot wake the supervisor against a stream that has not had a chance to connect yet. Reopening backs off from the second consecutive attempt on, doubling from 1s up to the tick interval, so a stream that fails as soon as it opens cannot spin the loop — the worst case is where every failure used to sit. Liveness is read after that wait rather than before, leaving alone a stream that a lifecycle or network trigger reopened in the meantime.
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
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.
Five defects behind the reported late messages in chat and the unread badge that survives opening a conversation. The first two delay delivery; the rest lose a read the client has already taken.
Stream liveness stayed
truethrough the whole retry windowisStreamActiveonBidirectionalStreamReferenceis set byactivateStream()on a first response and cleared only by an explicit teardown.openBidirectionalStreamretries internally —collectionJob.cancel(); requestChannel.close(); continue— and never touches the reference, so after a retryable failure the flag reported a live stream while nothing was connected.Every external recovery trigger reads that flag, or the reference's mere existence:
EventStreamDelegatechecksisStreamActiveopen(), called from lifecycle resume and the debounced network-reconnect collector, checksisConnected(streamRef != null)With a stale
trueall of them no-op, so delivery resumed only when the loop's own backoff got lucky, or when it exhausted its eight attempts — roughly 91s of sleeps at the event stream's 1s base delay — andonErrorfinally nulled the reference so the next heartbeat tick could reopen.The loop now marks the reference down at the top of each attempt, so liveness describes the stream that exists rather than the last one that ever connected.
deactivateStream()is separate fromcancel()/destroy()because both of those cancel the scope the reconnect loop itself runs on.Nothing noticed a stream that stopped retrying
The reconnect loop retries
UNAVAILABLE,CANCELLEDandABORTED. Anything else —INTERNAL,UNKNOWN, the ping timeout that fires when a stream goes silent — ends the loop and callsonError, which clears the reference and leaves the connection down. The only thing watching forthat was the heartbeat's 30s liveness poll, so a stream could die a second after a tick and stay
dead for the rest of the interval, with every message in that window arriving late.
EventStreamingControllernow emits onstreamFailureswhen it clears the reference, and theheartbeat waits on that signal or its tick, whichever comes first. A pending signal is dropped when
a stream is opened, so it cannot wake the supervisor against a stream that has not had a chance to
connect yet.
Reopening backs off from the second consecutive attempt on, doubling from 1s up to the tick
interval, so a stream that fails as soon as it opens cannot spin the loop — the worst case is where
every failure used to sit. Liveness is read after that wait rather than before, leaving alone a
stream that a lifecycle or network trigger reopened in the meantime.
A feed sync rewound the local read pointer
ChatMemberDao.upsertwas a whole-row@Insert(REPLACE), soperformFeedSyncoverwrotepointers_jsonwith the server's copy. The READ pointer lives in that column and is advanced locally as messages come into view, then reported to the server afterwards — so a sync racing that report, or following a failed one, rewound the pointer and the chat re-reported as unread.ChatMetadataDao.upsertalready avoids exactly this forlatest_event_sequenceandanalytics_counted_through.pointers_jsonis the same kind of client-owned watermark and now merges by taking the greater value per pointer, which is safe because a pointer only ever moves forward on either side. No entity or schema change, so no migration.The
FullRefreshbranch of the event stream deleted a chat's member rows before re-inserting them, which defeated any merge. It prunes departed members instead.The two symptoms compound: a dead stream makes the heartbeat fire a feed sync every 30s, and each one was a chance to rewind the pointer.
A read pointer was dropped when the member row was missing
ChatMemberDao.updatePointerswas a bareUPDATE ... WHERE chat_id_hex = ? AND user_id_hex = ?, which matches nothing when the row is absent, so the pointer went nowhere and nothing reported it. Both callers can reach it in that state: a read is written the moment a message is on screen, which can beat the feed sync that writes the membership, and the event stream applies pointer updates for members the feed has not written yet.It is now
advancePointer, which inserts the row when it is missing and otherwise carries the member's other pointers over. It is also forward-only, on the same reasoning as the merge above: the stream appliespointerUpdatesas they arrive, including the server's copy of a member's own pointer, which can sit behind a local advance that has not been reported yet. A row written this way has nouser_profilesentry yet; the read mapper already tolerates that and a later sync fills it in. The read-merge-write moved into the DAO with it, so a feed sync landing between the read and the write can no longer be lost.A failed read report was never retried
advanceReadPointerwrites the pointer locally and then reports it, and the report isfire-and-forget:
ChatViewModeldrops theResultand nothing retries. With the local write nowdurable, a failed report leaves the two copies apart for good — this device's badge clears while
the server still considers the chat unread, which keeps sending pushes for messages already read
and shows them unread on every other device and after a reinstall.
A feed payload carries the server's own copy of the pointer, so the sync is where the two can be
compared. Members are merged first, making the stored value
max(local, server); when it is aheadof what the feed returned, the local advance never landed, and the sync emits
ReadPointerUnreportedfor the coordinator to re-send. Riding the existing sync triggers — login,foreground, network reconnect, heartbeat — rather than retrying in place also recovers a report
lost to a process death.
The re-report calls the controller directly instead of going back through
advanceReadPointer:the local write and the message-received analytics already happened when the message was seen.
Tests
ChatMemberDaoTestis new and covers what a sync may overwrite on a member row, and what an advance does when the member has not synced yet or is already further ahead. The added case inOpenBidirectionalStreamTestactivates a stream, fails it withUNAVAILABLE, and asserts the reference reports inactive while the loop is parked waiting on the retry.EventStreamHeartbeatTestcovers the supervisor: a failure signal reopens well inside the tick, a stream that keeps failing is spaced out by the backoff, and the periodic tick still reopens a dead stream on its own.FeedSyncReadPointerReportTestsyncs a feed whose pointer sits behind the stored one and asserts the re-report is emitted, and not emitted when the server is level.