Chat race conditions #474

Manually merged
hloth merged 1 commit from Enucatl/open-grind:fix/chat-snapshot-races into main 2026-10-16 13:40:48 +02:00
Contributor
  1. A delayed snapshot removes a new message.
    The app reconnects and requests chat history. While that request is pending, a live message arrives: “Are you nearby?” The response then arrives with older history that excludes it. Previously, the response could remove the message and replace the inbox preview with older text. The fix keeps messages that arrived after the request started.

  2. Recovery produces the wrong unread count.
    You miss five messages while offline. During recovery, a sixth arrives live. The delayed snapshot reports five unread; the local state knows about only the sixth. Taking the larger count gives five instead of six. The reverse can happen when another device reads earlier messages: the app keeps too many unread. The fix preserves live activity and requests a fresh snapshot to obtain the correct count.

  3. Old work restarts after switching accounts.
    Account A has an inbox request pending. A message for an unloaded conversation joins that request. You switch to account B, destroying A’s state. When A’s request finishes, its waiting message handler could interpret the missing conversation as a reason to retry. That retry uses B’s current credentials and can update A’s retired state or invoke notification callbacks. The fix invalidates pending requests and stops their continuations after destruction.

1. A delayed snapshot removes a new message. The app reconnects and requests chat history. While that request is pending, a live message arrives: “Are you nearby?” The response then arrives with older history that excludes it. Previously, the response could remove the message and replace the inbox preview with older text. The fix keeps messages that arrived after the request started. 2. Recovery produces the wrong unread count. You miss five messages while offline. During recovery, a sixth arrives live. The delayed snapshot reports five unread; the local state knows about only the sixth. Taking the larger count gives five instead of six. The reverse can happen when another device reads earlier messages: the app keeps too many unread. The fix preserves live activity and requests a fresh snapshot to obtain the correct count. 3. Old work restarts after switching accounts. Account A has an inbox request pending. A message for an unloaded conversation joins that request. You switch to account B, destroying A’s state. When A’s request finishes, its waiting message handler could interpret the missing conversation as a reason to retry. That retry uses B’s current credentials and can update A’s retired state or invoke notification callbacks. The fix invalidates pending requests and stops their continuations after destruction.
Keep messages and inbox activity that arrive during recovery fetches.
Queue another snapshot when live updates make unread counts ambiguous.

Invalidate inbox work on destruction and stop delayed missing-chat
requests from restarting fetches or invoking callbacks after sign-out.

Add regressions for these races and keep Chromium shadow compositing
consistent in the browser screenshot comparisons.
Remove explanatory comment from browser helper
Some checks failed
check / check (pull_request) Has been cancelled
credits / credits (pull_request) Has been cancelled
362ff376e5
hloth requested changes 2026-10-15 22:08:10 +02:00
Dismissed
hloth left a comment

Also squash commits and force push

Also squash commits and force push
@ -10,3 +10,3 @@
dim: '[data-slot="live-stack-dim"]',
};
const NO_EDGE = "0 0 #0000";
const NO_EDGE = "-1px 0 0 rgb(255 255 255 / 0.001)";
Owner

Unrelated to chat, move to a separate PR with the spec it fixes

Unrelated to chat, move to a separate PR with the spec it fixes
Author
Contributor

moved to #483

moved to #483
Enucatl marked this conversation as resolved
@ -185,3 +185,3 @@
if (this.#seenMessages.has(message.messageId)) {
if (!this.#isActive(conversationId))
if (conversationId !== this.#activeConversationId)
Owner

Why inline #isActive?

Why inline `#isActive`?
Author
Contributor

reverted

reverted
Enucatl marked this conversation as resolved
Lines 223-225
@ -222,3 +223,3 @@
const isInboxPageRoot = page.route.id === "/(protected)/chat";
const twoColLayout = !singleColumnLayout.current;
const isConversationsListVisible = isInboxPageRoot || twoColLayout;
const isConversationsListVisible =
page.route.id === "/(protected)/chat" ||
!singleColumnLayout.current;
Owner

Why isInboxPageRoot and twoColLayout were removed?

Why `isInboxPageRoot` and `twoColLayout` were removed?
Author
Contributor

reverted

reverted
Enucatl marked this conversation as resolved
@ -278,3 +282,1 @@
const windowFloor = reachedEnd
? Number.NEGATIVE_INFINITY
: oldestFetchedTs;
const windowFloor = reachedEnd ? -Infinity : oldestFetchedTs;
Owner

Number.NEGATIVE_INFINITY, as at 252 and across the codebase. Same at 288, 622

`Number.NEGATIVE_INFINITY`, as at 252 and across the codebase. Same at 288, 622
Author
Contributor

reverted

reverted
Enucatl marked this conversation as resolved
@ -334,3 +342,1 @@
}: {
errorLabel?: string;
}): Promise<boolean> {
async #runSyncLatest(args: { errorLabel?: string }): Promise<boolean> {
Owner

Why stop destructuring errorLabel?

Why stop destructuring `errorLabel`?
Author
Contributor

reverted

reverted
Enucatl marked this conversation as resolved
@ -436,3 +443,1 @@
const joinedSyncWasTooEarly =
joinedRunningSync && syncSucceeded && !this.#find(id);
if (joinedSyncWasTooEarly) await this.ensureLoaded(id);
if (joinedRunningSync && syncSucceeded && !this.#find(id))
Owner

joinedSyncWasTooEarly dropped

`joinedSyncWasTooEarly` dropped
Author
Contributor

reverted

reverted
Enucatl marked this conversation as resolved
@ -606,3 +607,1 @@
return this.entries.find(
(e) => e.data.conversationId === conversationId,
);
#find(id: string): Conversation | undefined {
Owner

Unrelated rename, revert

Unrelated rename, revert
Author
Contributor

reverted

reverted
Enucatl marked this conversation as resolved
Lines 611-615
@ -618,3 +611,5 @@
const { conversationId } = incoming.data;
this.#readRequests.noteSnapshot(conversationId);
mergeConversation({
#mergeIncoming(
existing: Conversation,
incoming: Conversation,
atStart: Map<string, number>,
): void {
Owner

Codebase uses named-params rule. You can accidentally swap existing and incoming. Also atStart/startedWith names

Codebase uses named-params rule. You can accidentally swap `existing` and `incoming`. Also `atStart`/`startedWith` names
Enucatl marked this conversation as resolved
@ -7,16 +7,29 @@ export function mergeConversation({
existing,
incoming,
pendingFlags,
lastActivityAtFetchStart = existing.data.lastActivityTimestamp,
Owner

Make it required, the default brings the old race back for any new caller

Make it required, the default brings the old race back for any new caller
Author
Contributor

required now

required now
Enucatl marked this conversation as resolved
@ -242,3 +243,1 @@
this.#syncCache();
return;
}
if (!changed) return this.#syncCache();
Owner

Unrelated, revert

Unrelated, revert
Author
Contributor

reverted

reverted
Enucatl marked this conversation as resolved
@ -144,15 +144,18 @@ export function sentMessages(
export function mergeServerMessages({
local,
server,
localIdsAtFetchStart = local.map((m) => m.messageId),
Owner

Make it required, the default brings the old race back for any new caller

Make it required, the default brings the old race back for any new caller
Author
Contributor

required now

required now
Enucatl marked this conversation as resolved
Enucatl force-pushed fix/chat-snapshot-races from 362ff376e5
Some checks failed
check / check (pull_request) Has been cancelled
credits / credits (pull_request) Has been cancelled
to 72460a981c
Some checks failed
credits / credits (pull_request) Failing after 1m2s
ci-orchestrator / provision running on cpx42 in fsn1
check / check (pull_request) Successful in 42m49s
2026-10-16 10:00:39 +02:00
Compare
hloth approved these changes 2026-10-16 13:33:35 +02:00
hloth manually merged commit 73aaaa9a56 into main 2026-10-16 13:40:48 +02:00
Owner

Thanks!

Thanks!
Enucatl deleted branch fix/chat-snapshot-races 2026-10-16 13:42:11 +02:00
Sign in to join this conversation.
No description provided.