Skip to content

channels: loadChannels() replaces the channel array and discards all client-only state #2095

Description

@efiten

loadChannels() replaces the channels array wholesale and carries nothing across. Two consequences, one root cause, and one of them needs no race at all.

This came out of reviewing #2087. That PR isolates the flaky assertion correctly and changes no production code, so #2086 closes green over what is described below. Opening this so the finding is not lost with it. Credit to @dborup, who raised the mechanism on #2087.

Root cause

public/channels.js:1661-1682:

const data = await api('/channels' + qs, { ttl: CLIENT_TTL.channels });
channels = (data.channels || []).map(ch => {
  ch.lastActivityMs = ch.lastActivity ? new Date(ch.lastActivity).getTime() : 0;
  return ch;
}).sort((a, b) => (b.lastActivityMs || 0) - (a.lastActivityMs || 0));
renderChannelList();
reconcileSelectionAfterChannelRefresh();

Whole-array replacement with the server snapshot. Every client-only field on those objects is discarded, and nothing merges anything back.

messages already has protection for exactly this: mergeWsAppendedIntoRest() at channels.js:34-57, added in #1498. channels never got the equivalent.

1. Region filter or the show-encrypted toggle destroys client-only channel state

No timing involved. mergeUserChannels() is called at channels.js:477, :1069 and :1144. That last one is inside the .then() of the loadChannels() at :1137, which is init. The other two loadChannels() call sites do not re-run it:

  • channels.js:857 — RegionFilter.onChange. public/region-filter.js:86 toggleRegion only invokes listeners; nothing re-initialises the page.
  • channels.js:1154 — the mc-channels-show-encrypted-changed customizer toggle.

What is lost on either action:

Lost Set only at Effect
user:* PSK rows channels.js:532 The My Channels section disappears: renderChannelList derives it as channels.filter(c => c.userAdded === true) at :1998, and the server returns neither userAdded nor a user:-prefixed hash
userAdded / userLabel on server-known channels channels.js:518-529 Remove and share controls and the user's own label vanish from those rows
ch.unread channels.js:1602 only Every unread badge resets to 0 (:1760 reads ch.unread)

Worst case: if the selected channel was a user:* row, reconcileSelectionAfterChannelRefresh() (channels.js:133-146, called at :1675) does not find selectedHash in the refreshed list, so it nulls selectedHash, sets messages = [], rewrites the URL to #/channels, and replaces the open conversation with "Choose a channel from the sidebar".

To reproduce: open a PSK channel, then change the region filter or toggle show-encrypted. You are thrown out of the open conversation and My Channels is gone.

2. A WebSocket update landing during the initial load is dropped or reverted

This is the race #2086 surfaced. init() is not async: it fires loadChannels() at channels.js:1137, the await api('/channels') yields, and init() continues synchronously to register the WS handler at :1589. So the handler is live for the whole in-flight duration of the request.

processWSBatch either mutates a found entry or pushes a new one (channels.js:1450-1463). Both are then overwritten at :1670:

  • A public channel the server already knows keeps its row, but the WS-applied lastSender, lastMessage, lastActivityMs and messageCount revert to the snapshot.
  • A channel not in the response loses its pushed row entirely. The window is wider than a network round trip because api('/channels', { ttl: CLIENT_TTL.channels }) can serve a client-cached body up to 15s old (public/app.js:99, :105), and a 15s-stale snapshot cannot contain a channel whose first packet arrived two seconds ago.

Messages are not at risk here: selectedHash is still null until selectChannel() runs inside the .then() at :1146, so the if (selectedHash && ...) branch at :1465 cannot fire during the window.

This one self-heals. The next packet on that channel re-pushes the row, and the next WS batch overwrites the reverted preview.

Severity

Finding 1 is the one to fix first: it needs no race, it is reproducible on demand, and it closes an open conversation. Finding 2 is a flicker that repairs itself.

Suggested shape

One merge helper, mirroring mergeWsAppendedIntoRest, applied inside loadChannels() before renderChannelList() and reconcileSelectionAfterChannelRefresh():

  • re-run mergeUserChannels() so user:* rows and userAdded/userLabel survive the replacement
  • carry unread across by hash
  • keep WS-fresher lastActivityMs / lastSender / lastMessage when they are newer than the snapshot

Per AGENTS.md rule 1 that needs a unit test for the helper plus a regression test for the reconcile eviction. The helper should take its inputs as parameters so it is testable in isolation, the way mergeWsAppendedIntoRest does.

Not verified

Both findings are read from the code and traced through every call site. Neither was reproduced in a browser.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions