mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(desktop): make echo reconciliation ownership-aware so concurrent identical sends both survive
Wren's redteam blocker on #3329: two in-flight replies with the same author/content/root are semantically indistinguishable to isMatchingPendingMessage, so under the ordering echo1 -> success1 -> echo2 -> success2 the second echo claimed the FIRST send's already-consumed localKey, and the localKey-keyed dedupe then evicted the first confirmed reply — an accepted user message silently deleted from the pane. reconcileIncomingMessage now resolves the incoming event's render key ownership-aware (resolveIncomingLocalKey): 1. a confirmed row with the same id already owns a key -> inherit it, never re-match a pending; 2. a caller-supplied localKey (onSuccess binding) is honored only if no OTHER confirmed row owns it — if an echo claimed that pending first, fall through and claim a remaining pending instead; 3. otherwise claim the first semantically matching pending row. Pending and confirmed rows never share a key (claiming a pending removes it in the same merge), so a claimed key can never evict a confirmed row. Regression: concurrent-identical-replies ordering matrix over all 4! echo/success interleavings, asserting both accepted events survive with distinct render keys and no pending residue. Proven red against the previous merge logic (stash-verified) and green with the fix. Full desktop unit suite 3728/3728; both thread-liveness Playwright smoke specs pass. Co-authored-by: Tyler Longwell <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
This commit is contained in:
co-authored by
Tyler Longwell
parent
6b7dc42001
commit
6c3d472ffb
@@ -31,20 +31,64 @@ function isMatchingPendingMessage(pending: RelayEvent, incoming: RelayEvent) {
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the render key (localKey) the incoming event should carry, without
|
||||
* ever colliding with a confirmed row's key. Concurrent identical sends make
|
||||
* pending rows semantically indistinguishable, so key assignment must be
|
||||
* ownership-aware or a later echo/success evicts the other send's confirmed
|
||||
* row (the concurrent-identical-replies blocker):
|
||||
*
|
||||
* 1. A confirmed row with the same id already exists → the event was
|
||||
* reconciled once (its echo or REST success won the race); inherit that
|
||||
* row's key and never re-match a pending.
|
||||
* 2. The caller supplied a localKey (onSuccess binding) and no *other*
|
||||
* confirmed row owns it → honor it. If a different confirmed row owns
|
||||
* it (an echo claimed that pending first), fall through and claim a
|
||||
* remaining pending instead.
|
||||
* 3. Otherwise claim the first semantically matching pending row.
|
||||
*
|
||||
* Pending and confirmed rows never share a key (claiming a pending removes
|
||||
* it in the same merge), so a claimed pending key cannot evict a confirmed
|
||||
* row.
|
||||
*/
|
||||
function resolveIncomingLocalKey(
|
||||
normalizedCurrent: RelayEvent[],
|
||||
incoming: RelayEvent,
|
||||
): string | undefined {
|
||||
const confirmedExisting = normalizedCurrent.find(
|
||||
(message) => message.id === incoming.id && !message.pending,
|
||||
);
|
||||
if (confirmedExisting) {
|
||||
return getLocalRenderKey(confirmedExisting);
|
||||
}
|
||||
|
||||
const supplied = incoming.localKey;
|
||||
if (supplied != null) {
|
||||
const ownedByOtherConfirmed = normalizedCurrent.some(
|
||||
(message) =>
|
||||
!message.pending &&
|
||||
message.id !== incoming.id &&
|
||||
getLocalRenderKey(message) === supplied,
|
||||
);
|
||||
if (!ownedByOtherConfirmed) {
|
||||
return supplied;
|
||||
}
|
||||
}
|
||||
|
||||
const replacedPending = normalizedCurrent.find((message) =>
|
||||
isMatchingPendingMessage(message, incoming),
|
||||
);
|
||||
return replacedPending ? getLocalRenderKey(replacedPending) : undefined;
|
||||
}
|
||||
|
||||
export function reconcileIncomingMessage(
|
||||
current: RelayEvent[],
|
||||
incoming: RelayEvent,
|
||||
): RelayEvent[] {
|
||||
const normalizedCurrent = dedupeMessagesById(current);
|
||||
const replacedPending = normalizedCurrent.find((message) =>
|
||||
isMatchingPendingMessage(message, incoming),
|
||||
);
|
||||
const incomingWithLocalKey = replacedPending
|
||||
? {
|
||||
...incoming,
|
||||
localKey: replacedPending.localKey ?? replacedPending.id,
|
||||
}
|
||||
: incoming;
|
||||
const localKey = resolveIncomingLocalKey(normalizedCurrent, incoming);
|
||||
const incomingWithLocalKey =
|
||||
localKey === incoming.localKey ? incoming : { ...incoming, localKey };
|
||||
const incomingLocalKey = getLocalRenderKey(incomingWithLocalKey);
|
||||
const deduped = normalizedCurrent.filter(
|
||||
(message) =>
|
||||
|
||||
@@ -218,7 +218,99 @@ test("reconcileSentThreadReply_echoArrivedFirst_staysSingleEntry", () => {
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// 6. Error rollback: failed pending goes, in-flight live replies stay
|
||||
// 6. Concurrent identical replies: every echo/success interleaving keeps both
|
||||
// ---------------------------------------------------------------------------
|
||||
//
|
||||
// Two in-flight replies with the same author/content/root are semantically
|
||||
// indistinguishable, so echo reconciliation must never let a later event
|
||||
// steal a key already owned by a confirmed row (that eviction was the
|
||||
// concurrent-identical-replies blocker: echo1 → success1 → echo2 → success2
|
||||
// left only real-2 in the cache). Each success and each echo may arrive in
|
||||
// any order relative to the others, so we run the full 4! ordering matrix.
|
||||
|
||||
test("concurrentIdenticalReplies_allEchoSuccessOrderings_keepBothAcceptedEvents", () => {
|
||||
const operationNames = ["echo1", "success1", "echo2", "success2"];
|
||||
|
||||
function permutations(items) {
|
||||
if (items.length <= 1) return [items];
|
||||
return items.flatMap((item, index) =>
|
||||
permutations([...items.slice(0, index), ...items.slice(index + 1)]).map(
|
||||
(rest) => [item, ...rest],
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
for (const ordering of permutations(operationNames)) {
|
||||
const queryClient = new QueryClient();
|
||||
queryClient.setQueryData(channelMessagesKey(CHANNEL_ID), [
|
||||
makeEvent({ id: "root-1", content: "thread root" }),
|
||||
]);
|
||||
const pending1 = makePendingReply(queryClient, "root-1", "same reply");
|
||||
const threadKey = insertPendingThreadReply(
|
||||
queryClient,
|
||||
CHANNEL_ID,
|
||||
pending1,
|
||||
);
|
||||
const pending2 = makePendingReply(queryClient, "root-1", "same reply");
|
||||
insertPendingThreadReply(queryClient, CHANNEL_ID, pending2);
|
||||
|
||||
const confirmed = {
|
||||
1: makeEvent({ id: "real-1", parentId: "root-1", content: "same reply" }),
|
||||
2: makeEvent({ id: "real-2", parentId: "root-1", content: "same reply" }),
|
||||
};
|
||||
const operations = {
|
||||
echo1: () =>
|
||||
queryClient.setQueryData(
|
||||
threadKey,
|
||||
mergeMessages(queryClient.getQueryData(threadKey), confirmed[1]),
|
||||
),
|
||||
echo2: () =>
|
||||
queryClient.setQueryData(
|
||||
threadKey,
|
||||
mergeMessages(queryClient.getQueryData(threadKey), confirmed[2]),
|
||||
),
|
||||
success1: () =>
|
||||
reconcileSentThreadReply(
|
||||
queryClient,
|
||||
threadKey,
|
||||
pending1.id,
|
||||
confirmed[1],
|
||||
),
|
||||
success2: () =>
|
||||
reconcileSentThreadReply(
|
||||
queryClient,
|
||||
threadKey,
|
||||
pending2.id,
|
||||
confirmed[2],
|
||||
),
|
||||
};
|
||||
|
||||
for (const name of ordering) {
|
||||
operations[name]();
|
||||
}
|
||||
|
||||
const label = ordering.join(" → ");
|
||||
const cached = queryClient.getQueryData(threadKey);
|
||||
assert.deepEqual(
|
||||
cached.map((event) => event.id).sort(),
|
||||
["real-1", "real-2"],
|
||||
`${label}: both accepted events must survive`,
|
||||
);
|
||||
assert.ok(
|
||||
cached.every((event) => !event.pending),
|
||||
`${label}: no pending rows may remain`,
|
||||
);
|
||||
const localKeys = new Set(cached.map((event) => event.localKey));
|
||||
assert.equal(
|
||||
localKeys.size,
|
||||
2,
|
||||
`${label}: confirmed rows must keep distinct render keys`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// 7. Error rollback: failed pending goes, in-flight live replies stay
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
test("rollbackPendingThreadReply_removesPendingKeepsInFlightLiveReplies", () => {
|
||||
|
||||
Reference in New Issue
Block a user