fix(read-state): O(n) eviction, fitsAfterTrim signal, suppress publish on overflow

Address two IMPORTANT findings from Thufir's review of #1305:

1. O(n²) re-encode loop — the eviction loop called encoder.encode() on
   every iteration, O(n) per step × n steps = O(n²). With 7,500 entries
   this caused a confirmed 4.7s UI freeze on first publish after upgrade.
   Replace with an O(n) pass: compute total bytes once, subtract each
   entry's per-entry delta (key.length + 4 + timestamp digits), collect
   entries to evict, delete them in a batch, then do one final encode as
   the authoritative check (handles JSON comma-accounting edge cases the
   delta estimate ignores).

2. Silent failure when channel-only blob exceeds budget — after exhausting
   all msg:/thread: entries the function returned without checking whether
   the remaining blob still exceeded the budget. The caller would then
   proceed to publish a blob the relay would reject.
   Fix: trimContextsToBudget() now returns { evicted, fitsAfterTrim }.
   currentContexts() returns null when fitsAfterTrim is false; publish()
   and initialize() both guard against null and skip the publish.

Also address two MINOR findings:
- JSDoc now documents that trimContextsToBudget mutates contexts in place
- Empty-map test replaced with a test that actually exercises the
  fitsAfterTrim=false path (channel-only blob smaller than budget)

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

Author: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7

desktop/src/features/channels/readState/readStateManager.test.mjs

@@ -156,8 +156,9 @@ const THREAD_ID = "b".repeat(64);
 test("trimContextsToBudget_underBudget_returnsZeroAndLeavesContextsUnchanged", () => {
   const contexts = { [`msg:${MSG_ID}`]: 100 };
   // A very large budget — nothing should be evicted.
-  const evicted = trimContextsToBudget(contexts, CLIENT_ID, 1_000_000);
+  const { evicted, fitsAfterTrim } = trimContextsToBudget(contexts, CLIENT_ID, 1_000_000);
   assert.equal(evicted, 0);
+  assert.equal(fitsAfterTrim, true);
   assert.ok(`msg:${MSG_ID}` in contexts);
 });
 
@@ -176,8 +177,9 @@ test("trimContextsToBudget_overBudget_evictsMsgEntriesOldestFirst", () => {
   // Budget that requires evicting at least one entry.
   const budget = fullSize - 10;
 
-  const evicted = trimContextsToBudget(contexts, CLIENT_ID, budget);
+  const { evicted, fitsAfterTrim } = trimContextsToBudget(contexts, CLIENT_ID, budget);
   assert.ok(evicted >= 1, `expected at least 1 eviction, got ${evicted}`);
+  assert.equal(fitsAfterTrim, true);
   // The oldest entry (ts=1) must be gone.
   assert.ok(!(`msg:${MSG_ID}` in contexts), "oldest msg entry should be evicted");
   // Result must fit within budget.
@@ -201,10 +203,11 @@ test("trimContextsToBudget_channelKeysNeverEvicted", () => {
   ).length;
   const budget = Math.floor(fullSize / 2);
 
-  trimContextsToBudget(contexts, CLIENT_ID, budget);
+  const { fitsAfterTrim } = trimContextsToBudget(contexts, CLIENT_ID, budget);
 
   // Channel key must survive regardless of how many msg entries were evicted.
   assert.ok("channel:some-channel-id" in contexts, "channel key must not be evicted");
+  assert.equal(fitsAfterTrim, true);
   const resultSize = encoder.encode(
     JSON.stringify({ v: 1, client_id: CLIENT_ID, contexts }),
   ).length;
@@ -232,14 +235,37 @@ test("trimContextsToBudget_msgEvictedBeforeThread", () => {
   ).length;
   const budget = oneEntrySize + 5; // fits one entry, not two
 
-  const evicted = trimContextsToBudget(contexts, CLIENT_ID, budget);
+  const { evicted, fitsAfterTrim } = trimContextsToBudget(contexts, CLIENT_ID, budget);
   assert.equal(evicted, 1);
+  assert.equal(fitsAfterTrim, true);
   assert.ok(!(`msg:${MSG_ID}` in contexts), "msg entry should be evicted before thread");
   assert.ok(`thread:${THREAD_ID}` in contexts, "thread entry should survive");
 });
 
-test("trimContextsToBudget_emptyContexts_returnsZero", () => {
+test("trimContextsToBudget_emptyContexts_returnsZeroAndFits", () => {
+  // Empty contexts: blob is just the skeleton — fits any reasonable budget.
   const contexts = {};
-  const evicted = trimContextsToBudget(contexts, CLIENT_ID, 100);
+  const { evicted, fitsAfterTrim } = trimContextsToBudget(contexts, CLIENT_ID, 1_000_000);
   assert.equal(evicted, 0);
+  assert.equal(fitsAfterTrim, true);
+});
+
+test("trimContextsToBudget_channelOnlyBlobExceedsBudget_fitsAfterTrimFalse", () => {
+  // Channel keys cannot be evicted. If the channel-only skeleton exceeds the
+  // budget, fitsAfterTrim must be false so the caller can suppress the publish.
+  const contexts = {
+    "channel:some-channel-id": 100,
+  };
+  const encoder = new TextEncoder();
+  const skeletonSize = encoder.encode(
+    JSON.stringify({ v: 1, client_id: CLIENT_ID, contexts }),
+  ).length;
+  // Budget smaller than the channel-only skeleton — cannot be satisfied.
+  const budget = skeletonSize - 1;
+
+  const { evicted, fitsAfterTrim } = trimContextsToBudget(contexts, CLIENT_ID, budget);
+  assert.equal(evicted, 0, "no evictable entries exist");
+  assert.equal(fitsAfterTrim, false, "channel-only blob still exceeds budget");
+  // Channel key must still be present.
+  assert.ok("channel:some-channel-id" in contexts);
 });

desktop/src/features/channels/readState/readStateManager.ts

@@ -123,25 +123,40 @@ export function applyRemoteContextTimestamp(args: {
   return result;
 }
 
+/**
+ * Result of a `trimContextsToBudget` call.
+ */
+export interface TrimResult {
+  /** Number of entries removed from `contexts`. */
+  evicted: number;
+  /** True when the serialized blob fits within `maxBytes` after trimming. */
+  fitsAfterTrim: boolean;
+}
+
 /**
  * Trim a contexts map to fit within `maxBytes` when serialized as the JSON
  * blob `{v:1, client_id, contexts}`. Evicts oldest `msg:` entries first
  * (lowest timestamp), then oldest `thread:` entries. Channel keys are never
- * evicted. Returns the number of entries removed.
+ * evicted. Mutates `contexts` in place.
+ *
+ * Returns `{ evicted, fitsAfterTrim }`. `fitsAfterTrim` is false when the
+ * remaining blob (channel keys only) still exceeds `maxBytes` — the caller
+ * must not publish in that case.
  *
  * Exported for unit testing; callers should prefer `currentContexts()`.
  */
 export function trimContextsToBudget(
   contexts: Record<string, number>,
   clientId: string,
   maxBytes: number,
-): number {
+): TrimResult {
   const encoder = new TextEncoder();
   const blobFor = (c: Record<string, number>) =>
     JSON.stringify({ v: 1, client_id: clientId, contexts: c });
 
-  if (encoder.encode(blobFor(contexts)).length <= maxBytes) {
-    return 0;
+  let currentBytes = encoder.encode(blobFor(contexts)).length;
+  if (currentBytes <= maxBytes) {
+    return { evicted: 0, fitsAfterTrim: true };
   }
 
   const msgEntries: [string, number][] = [];
@@ -157,13 +172,26 @@ export function trimContextsToBudget(
   msgEntries.sort((a, b) => a[1] - b[1]);
   threadEntries.sort((a, b) => a[1] - b[1]);
 
-  let evicted = 0;
-  for (const [key] of [...msgEntries, ...threadEntries]) {
-    if (encoder.encode(blobFor(contexts)).length <= maxBytes) break;
+  // O(n) pass: subtract each entry's byte contribution from currentBytes and
+  // collect entries to evict. The per-entry estimate is `,"key":timestamp`
+  // (key.length + 3 bytes for `"`, `"`, `:` plus 1 comma) + timestamp digits.
+  // This is an approximation — the final encode below is the authoritative check.
+  const toEvict: string[] = [];
+  for (const [key, ts] of [...msgEntries, ...threadEntries]) {
+    if (currentBytes <= maxBytes) break;
+    // Contribution: `,"key":timestamp` — comma + quoted key + colon + value
+    currentBytes -= key.length + 3 + String(ts).length + 1;
+    toEvict.push(key);
+  }
+
+  for (const key of toEvict) {
     delete contexts[key];
-    evicted++;
   }
-  return evicted;
+
+  // Final authoritative check — handles JSON comma-accounting edge cases
+  // (e.g. last-entry comma disappears) that the per-entry estimate ignores.
+  const fitsAfterTrim = encoder.encode(blobFor(contexts)).length <= maxBytes;
+  return { evicted: toEvict.length, fitsAfterTrim };
 }
 
 export class ReadStateManager {
@@ -207,7 +235,8 @@ export class ReadStateManager {
     if (this.destroyed) return;
     await this.startLiveSubscription();
     if (this.destroyed) return;
-    if (!this.isIdenticalToLastPublished(this.currentContexts())) {
+    const initContexts = this.currentContexts();
+    if (initContexts !== null && !this.isIdenticalToLastPublished(initContexts)) {
       this.schedulePublish();
     }
 
@@ -543,8 +572,8 @@ export class ReadStateManager {
     // Build blob from contexts this client is allowed to publish.
     const contexts = this.currentContexts();
 
-    // Suppress no-op publishes
-    if (this.isIdenticalToLastPublished(contexts)) return;
+    // Suppress no-op publishes; also skip if the blob cannot fit within budget.
+    if (contexts === null || this.isIdenticalToLastPublished(contexts)) return;
 
     const blob: ReadStateBlob = {
       v: 1,
@@ -630,7 +659,7 @@ export class ReadStateManager {
     return true;
   }
 
-  private currentContexts(): Record<string, number> {
+  private currentContexts(): Record<string, number> | null {
     const contexts: Record<string, number> = {};
     for (const [ctx, ts] of this.effectiveState) {
       if (!this.publishableContextIds.has(ctx)) {
@@ -643,7 +672,7 @@ export class ReadStateManager {
     // the wrong metric — the relay rejects on byte size, not entry count. Evict
     // oldest msg: entries first (lowest timestamp), then thread: entries, until
     // the JSON fits. Channel keys are never evicted.
-    const evicted = trimContextsToBudget(
+    const { evicted, fitsAfterTrim } = trimContextsToBudget(
       contexts,
       this.clientId,
       READ_STATE_MAX_PLAINTEXT_BYTES,
@@ -653,6 +682,12 @@ export class ReadStateManager {
         `[ReadStateManager] currentContexts trimmed ${evicted} entries to fit byte budget`,
       );
     }
+    if (!fitsAfterTrim) {
+      console.error(
+        "[ReadStateManager] currentContexts: blob exceeds byte budget even after full eviction — skipping publish",
+      );
+      return null;
+    }
 
     return contexts;
   }