From 021e55f3f82e6fe9daa4624fb1443d5dfba19f2e Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 2 Sep 2026 15:31:28 -0400 Subject: [PATCH] fix(sync): thread preservedKey through per-record fold in readOutboxWithMeta MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without bestKey in the per-record reduce, folding two durable outbox records totaling 501 entries evicts the clicked channel before the final bound can protect it. Thread bestKey through merge(acc, r.store, bestKey) on every step so the reservation is upheld at each intermediate capacity-bound merge. Update the merge callback signature from (a, b) to (a, b, preservedKey?) — both mergeStores implementations already accept the optional third arg. Also fix two test gaps from Thufir pass 3: reconnect test: both the reconnect-callback fetch and the subsequent pre-publish fetch must return the remote 500-entry head so that reverting the hook reconnect to publish*(pending) triggers the 501-entry merge and fails. The previous single-return-on-call-1 mock left the pre-publish fetch returning [] — no 501-merge occurred and the wrong wiring stayed green. restart test: upgrade from one to two foreign-nonce outbox records. Record 1 carries the clicked channel + 499 entries with preservedKey; record 2 carries the 501st entry with no key. This exercises the per-record fold eviction (mutation c) directly: without bestKey in the reduce the fold evicts the click at the record-2 merge, before the final bound runs. The single-record test could not catch this class of defect. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../sidebar/lib/mergeLaneHook.shared.test.mjs | 102 ++++++++++++------ .../sidebar/lib/mergeLaneStorage.shared.ts | 7 +- 2 files changed, 73 insertions(+), 36 deletions(-) diff --git a/desktop/src/features/sidebar/lib/mergeLaneHook.shared.test.mjs b/desktop/src/features/sidebar/lib/mergeLaneHook.shared.test.mjs index 420119f7e3..e9ab893507 100644 --- a/desktop/src/features/sidebar/lib/mergeLaneHook.shared.test.mjs +++ b/desktop/src/features/sidebar/lib/mergeLaneHook.shared.test.mjs @@ -507,11 +507,14 @@ export function runMergeLaneHookSuite({ remoteHead.kind = 30078; const publishCalls = []; - let fetchCalls = 0; - relayClient.fetchEvents = async () => { - fetchCalls++; - return fetchCalls === 1 ? [remoteHead] : []; - }; + // Always return remoteHead so both the reconnect-callback fetch + // (fetchRemote*) AND the subsequent pre-publish fetch inside doPublish() + // see the 500-entry remote — together with the 500-entry pending store + // that is 501 entries, triggering the capacity-bound merge that evicts + // the clicked channel when pendingPreservedKey is reset. + // Without the fix (hook reverts to publish*(pending)), pendingPreservedKey + // is cleared before doPublish runs, so the 501-entry merge evicts the click. + relayClient.fetchEvents = async () => [remoteHead]; relayClient.publishEvent = async (evt) => { publishCalls.push(evt); }; @@ -531,7 +534,7 @@ export function runMergeLaneHookSuite({ let hook = null; try { - // Mount and let bootstrap settle + // Mount and let bootstrap settle (bootstrap fetch applies the remote head) await act(async () => { hook = renderHook(() => useHook(pubkey, relayUrl)); for (let i = 0; i < 20; i++) await Promise.resolve(); @@ -543,11 +546,12 @@ export function runMergeLaneHookSuite({ // Trigger the registered reconnect callback (exercises the real production seam). // The callback: fetchRemote*() → retryReconnect*Publish() → startCycle() → doPublish(). // retryReconnectPublish cancels the debounce and drives the cycle directly. + // doPublish then calls fetchRemote* again for the pre-publish merge, getting + // remoteHead (500 entries) → 501-entry mergeWithRemote → evicts click without key. assert.ok( typeof reconnect.cb === "function", "subscribeToReconnects must register a callback", ); - fetchCalls = 0; publishCalls.length = 0; await act(async () => { // Drive the reconnect callback's async chain: fetchRemote → retryReconnect. @@ -569,7 +573,8 @@ export function runMergeLaneHookSuite({ assert.ok( clickedId in published.channels, `clicked channel "${clickedId}" must survive 501-entry merge after reconnect — ` + - `reverting hook to publish*(pending) resets pendingPreservedKey and fails this`, + `reverting hook reconnect from retryReconnect*Publish() to publish*(pending) ` + + `resets pendingPreservedKey before doPublish runs and fails this`, ); hook.unmount(); @@ -585,16 +590,22 @@ export function runMergeLaneHookSuite({ // P3 restart: after a quit the prior window's outbox record is foreign (the // session nonce is gone). Bootstrap must recover the preservedKey from the - // foreign record and forward it to publishStars/publishMutes. Without the fix - // (readOutboxPreservedKey only finding own records), preservedKey is undefined - // and the 501-entry pre-publish merge evicts the clicked channel. + // foreign record and forward it to publishStars/publishMutes. + // + // This test uses TWO foreign records so it exercises both the per-record fold + // eviction (mutation c) and the isOwn-filter / bootstrap-forward mutations. + // Without bestKey threaded through the readOutboxWithMeta reduce, folding + // record 1 (500 entries) into record 2 (1 entry) totals 501 and evicts the + // click before the final bound can protect it. // // Failing mutations: // (a) Revert to readOutboxPreservedKey (isOwn filter): returns undefined for // the foreign record → publish with no key → clicked channel evicted. // (b) Drop bootstrap's preservedKey forward (publish(outbox, undefined)): // same eviction even if key was recovered. - test(`${label}: P3 restart — foreign-nonce outbox with preservedKey is recovered on bootstrap; clicked channel survives 501-entry merge`, async () => { + // (c) Remove bestKey from per-record reduce in readOutboxWithMeta: + // fold evicts click at record-2 merge before final bound runs. + test(`${label}: P3 restart — multi-record foreign-nonce outbox; preservedKey threads fold and bootstrap; clicked channel survives 501-entry merge`, async () => { const { act, cleanup, renderHook } = await import("@testing-library/react"); const { relayClient } = await import("@/shared/api/relayClient"); @@ -604,18 +615,57 @@ export function runMergeLaneHookSuite({ const pubkey = `pk-p3-restart-${label}`; const encodedRelay = encodeURIComponent(relayUrl); - // Local: clicked channel (oldest updatedAt=1) + 499 others at updatedAt=100 - const localChannels = { + // Record 1 (foreign, carries preservedKey): + // clicked channel (oldest updatedAt=1) + 499 base entries at updatedAt=100 + const record1Channels = { [clickedId]: { [entryValueField]: true, updatedAt: 1, rev: 1 }, }; for (let i = 0; i < MAX - 1; i++) { - localChannels[`ch-rs-${i}`] = { + record1Channels[`ch-rs-${i}`] = { [entryValueField]: false, updatedAt: 100, rev: 0, }; } - // Remote: same 499 + one fresh channel → merged = 501 + // Record 2 (foreign, no preservedKey): one additional entry. + // Folding record1 (500) + record2 (1 new) = 501 without bestKey → click evicted. + const record2Channels = { + [`ch-rs-extra-${label}`]: { + [entryValueField]: false, + updatedAt: 100, + rev: 0, + }, + }; + + // Seed local store (matches record1 so bootstrap sees the clicked channel) + window.localStorage.setItem( + storageKey(pubkey, relayUrl), + JSON.stringify({ version: 1, channels: record1Channels }), + ); + + // Seed TWO FOREIGN-NONCE outbox envelopes (models two prior window records after quit). + // Neither nonce matches this window's outboxWindowNonce(); both are foreign. + // Only record 1 carries preservedKey (max queuedAt → selected as bestKey). + const foreignKey1 = `${outboxKeyPrefix}:${pubkey}:${encodedRelay}:foreign-nonce-xyz:0000`; + window.localStorage.setItem( + foreignKey1, + JSON.stringify({ + store: { version: 1, channels: record1Channels }, + queuedAt: 1000, + preservedKey: clickedId, + }), + ); + const foreignKey2 = `${outboxKeyPrefix}:${pubkey}:${encodedRelay}:foreign-nonce-abc:0001`; + window.localStorage.setItem( + foreignKey2, + JSON.stringify({ + store: { version: 1, channels: record2Channels }, + queuedAt: 900, + // No preservedKey — second window did not click + }), + ); + + // Remote: same 499 base + one new → merged with resumed outbox = 501 const remoteChannels = {}; for (let i = 0; i < MAX - 1; i++) { remoteChannels[`ch-rs-${i}`] = { @@ -630,23 +680,6 @@ export function runMergeLaneHookSuite({ rev: 0, }; - // Seed local store - window.localStorage.setItem( - storageKey(pubkey, relayUrl), - JSON.stringify({ version: 1, channels: localChannels }), - ); - - // Seed a FOREIGN-NONCE outbox envelope (models the prior window's record after quit). - // The nonce "foreign-nonce-xyz" won't match this window's outboxWindowNonce(), - // so enumerateOutbox finds it as a non-own record. The envelope carries preservedKey. - const foreignKey = `${outboxKeyPrefix}:${pubkey}:${encodedRelay}:foreign-nonce-xyz:0000`; - const foreignEnvelope = JSON.stringify({ - store: { version: 1, channels: localChannels }, - queuedAt: 1000, - preservedKey: clickedId, - }); - window.localStorage.setItem(foreignKey, foreignEnvelope); - const restore = stubRelay(relayClient); const tauri = installEchoTauri(pubkey); const remoteHead = tauri.mintHead( @@ -702,7 +735,8 @@ export function runMergeLaneHookSuite({ assert.ok( clickedId in published.channels, `clicked channel "${clickedId}" must survive 501-entry merge after restart — ` + - `readOutboxPreservedKey with isOwn filter returns undefined for foreign record and fails this`, + `readOutboxPreservedKey with isOwn filter returns undefined for foreign record and fails this; ` + + `also fails when bestKey is dropped from per-record fold in readOutboxWithMeta`, ); hook.unmount(); diff --git a/desktop/src/features/sidebar/lib/mergeLaneStorage.shared.ts b/desktop/src/features/sidebar/lib/mergeLaneStorage.shared.ts index 75851baf62..042c144d60 100644 --- a/desktop/src/features/sidebar/lib/mergeLaneStorage.shared.ts +++ b/desktop/src/features/sidebar/lib/mergeLaneStorage.shared.ts @@ -197,7 +197,7 @@ export function readOutboxWithMeta( pubkey: string, relayUrl: string, parse: (json: unknown) => S | null, - merge: (a: S, b: S) => S, + merge: (a: S, b: S, preservedKey?: string) => S, defaultStore: S, bound: (s: S, preservedKey?: string) => S, ): OutboxWithMeta | null { @@ -227,8 +227,11 @@ export function readOutboxWithMeta( } // Fold all records and apply the capacity bound with the selected key so the // clicked channel is never evicted during the outbox read itself. + // bestKey is threaded through every per-record merge so that no intermediate + // merge — even across two durable records totaling 501 entries — evicts the + // reserved click before the final defensive bound. const merged = records.reduce( - (acc, r) => merge(acc, r.store), + (acc, r) => merge(acc, r.store, bestKey), defaultStore, ); return { store: bound(merged, bestKey), preservedKey: bestKey };