feat(scaling): serialize per-socket fan-out + NEW_CHANGES_BATCH (#7756 lever 3)

[JohnMcLear]

Summary

Honest re-evaluation after N=3 scoring: this PR's perf claim was wrong twice. The original "70% p95 drop at step 200" was a single-run outlier. The subsequent "tighter envelope" claim was a cross-matrix-entry comparison confounded by the runner-noise envelope (see scaling-dive doc for the methodology trap). Like-for-like comparison (develop-baseline vs this-PR-baseline, N=3 each):

Step	develop	this PR	takeaway
100	28/38/38	39/40/47	within noise (PR slightly worse)
200	30/37/51	37/50/59	within noise (PR slightly worse)
300	38/45/71	40/77/119	PR worse
350	39/39/122	63/109/131	PR worse
400	1758/2275/2463	1350/2373/3065	overlapping

The performance benefit isn't there. Recommendation now stands on the correctness fix only, not perf.

What this PR is for

Two bundled changes; only change A is justified by what we measured:

Change A — per-socket fan-out serialization (correctness fix). updatePadClients previously advanced sessioninfo.rev inside the collect phase, before the emit. Under concurrency that allowed overlapping fan-outs on the same socket; if the emit on one branch threw after the other branch had already advanced rev, revisions could be silently lost (the client enforces newRev === rev + 1 and stops applying on mismatch).

The fix snapshots startRev and headRev once and writes sessioninfo.rev = headRev immediately. A concurrent second run sees the bumped rev and skips the range; on emit failure sessioninfo.rev rolls back to startRev. At most one fan-out per socket per pad at a time, with retry semantics preserved. This is a real race the previous code exhibited.

Change B — NEW_CHANGES_BATCH wire format. When a recipient is more than one rev behind, the server packs queued revs into one NEW_CHANGES_BATCH emit. Single-rev fan-outs stay as NEW_CHANGES. Feature-flagged behind settings.newChangesBatch: false default; clients are forward-compatible.

In the dive, change B was dormant — steady-state catch-up is 1 rev per recipient per fan-out, so the batching branch never fired. It would fire under server slowness (GC pauses, disk hiccups, sustained delays inside updatePadClients). Useful forward-compat groundwork; not contributing to current measured numbers.

Tests

• src/tests/backend-new/specs/new-changes-batch.test.ts (4/4) — pins the wire-format decision via the exported buildNewChangesEmits function in src/node/handler/NewChangesPacker.ts (extracted so the test exercises the production code path).
• src/tests/backend-new/specs/prom-instruments.test.ts (5/5) — no regression on feat(metrics): 3 Prometheus counters for scaling dive (#7756) #7762.
• All 33 CI checks pass.

Recommendation

Merge for the correctness fix. The previous race was real and could lose revisions under concurrent commits. The perf benefit I claimed wasn't real — runner noise dominated my earlier measurements.

For the perf direction, the actually-measured win is #7774 (engine.io flush deferral), which tightens the tail at mid-load with N=3 confirmation. That PR doesn't depend on this one.

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Pack multi-revision fan-out into single NEW_CHANGES_BATCH emit

✨ Enhancement

Walkthroughs

Description

• Pack multiple queued revisions into single NEW_CHANGES_BATCH emit
  - Reduces engine.io packet count under high concurrency
  - WebSocket transport sends one frame per packet (polling already batches)
• Feature-flagged with settings.newChangesBatch (defaults to false)
  - Single-rev fan-outs remain as NEW_CHANGES (no overhead)
  - Multi-rev fan-outs use NEW_CHANGES_BATCH when enabled
• Clients forward-compatible: accept both message types
  - collab_client.ts and broadcast.ts normalize to list
  - Old clients connecting to enabled server will miss batched revisions
• Comprehensive test coverage with new test suite
  - 4 new tests pin wire-format decision
  - Existing prom-instruments tests remain green

Diagram

flowchart LR
  A["Multiple queued revisions"] --> B{"newChangesBatch enabled?"}
  B -->|No or single rev| C["Emit per-rev NEW_CHANGES"]
  B -->|Yes and multi-rev| D["Emit single NEW_CHANGES_BATCH"]
  C --> E["Client receives"]
  D --> E
  E --> F["Normalize to list"]
  F --> G["Apply revisions in order"]

Loading

File Changes

1. src/node/handler/PadMessageHandler.ts ✨ Enhancement +49/-20

Implement server-side revision batching logic

• Collect all pending revisions into array before emitting
• Emit NEW_CHANGES_BATCH when flag enabled and multiple revisions queued
• Fall back to per-revision NEW_CHANGES for single revisions or when flag disabled
• Moved error handling outside loop to wrap entire emit operation

src/node/handler/PadMessageHandler.ts

---

2. src/node/prom-instruments.ts ⚙️ Configuration changes +1/-0

Add NEW_CHANGES_BATCH to metrics allowlist

• Add NEW_CHANGES_BATCH to KNOWN_TYPES allowlist
• Enables Prometheus metric tracking for batched message type

src/node/prom-instruments.ts

---

3. src/node/utils/Settings.ts ⚙️ Configuration changes +16/-0

Add newChangesBatch feature flag setting

• Add newChangesBatch boolean field to SettingsType
• Set default to false to preserve legacy behavior
• Document feature purpose, client compatibility hazard, and rollout coordination requirement

src/node/utils/Settings.ts

---

View more (5)

4. src/static/js/broadcast.ts ✨ Enhancement +17/-11

Support NEW_CHANGES_BATCH in timeslider broadcast

• Accept both NEW_CHANGES and NEW_CHANGES_BATCH message types
• Normalize batched messages to array for uniform processing
• Apply each revision in order using existing changeset logic

src/static/js/broadcast.ts

---

5. src/static/js/collab_client.ts ✨ Enhancement +15/-8

Support NEW_CHANGES_BATCH in live editor client

• Accept both NEW_CHANGES and NEW_CHANGES_BATCH message types
• Normalize single-rev messages to array for uniform iteration
• Apply each revision in order within shared composition-safety await
• Update warning message to reference actual message type

src/static/js/collab_client.ts

---

6. src/static/js/types/SocketIOMessage.ts ✨ Enhancement +14/-0

Add TypeScript types for batched message format

• Define NewChangesItem type for individual revision in batch
• Define ClientNewChangesBatch type for batched message format
• Maintain backward compatibility with existing ClientNewChanges type

src/static/js/types/SocketIOMessage.ts

---

7. src/tests/backend-new/specs/new-changes-batch.test.ts 🧪 Tests +73/-0

Add unit tests for NEW_CHANGES_BATCH emit decision

• New test suite with 4 test cases covering emit decision logic
• Test flag off behavior (per-rev emissions)
• Test flag on with single revision (no batch overhead)
• Test flag on with multiple revisions (single batch emit)
• Test empty pending list edge case

src/tests/backend-new/specs/new-changes-batch.test.ts

---

8. settings.json.template 📝 Documentation +12/-0

Document newChangesBatch setting in template

• Add newChangesBatch configuration option with default false
• Document feature purpose and performance benefit
• Warn about client compatibility requirement during rollout

settings.json.template

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Test reimplements emit decision ✓ Resolved 📘 Rule violation ☼ Reliability

Description

The added test re-implements the NEW_CHANGES vs NEW_CHANGES_BATCH decision locally instead of
exercising the real server code path, so it would still pass even if the production batching logic
were removed or changed. This does not meet the requirement for a regression test that fails when
the fix is reverted.

Code

src/tests/backend-new/specs/new-changes-batch.test.ts[R16-36]

+// The decision the new code makes is small and pure: given a `pending`
+// array of N >= 1 revisions and the feature flag, emit one
+// NEW_CHANGES_BATCH (if N > 1 and flag on) or N NEW_CHANGES messages.
+// Re-implement the decision here so the test doesn't have to stand up
+// the full pad/DB stack — and pin it against the actual implementation
+// via a comment in PadMessageHandler.
+
+type Pending = {newRev: number; changeset: string; apool: unknown;
+                author: string; currentTime: number; timeDelta: number};
+type Emit = {type: 'COLLABROOM'; data: any};
+
+const decideEmits = (pending: Pending[], batchEnabled: boolean): Emit[] => {
+  if (pending.length === 0) return [];
+  if (batchEnabled && pending.length > 1) {
+    return [{type: 'COLLABROOM', data: {type: 'NEW_CHANGES_BATCH', changes: pending}}];
+  }
+  return pending.map((change) => ({
+    type: 'COLLABROOM',
+    data: {type: 'NEW_CHANGES', ...change},
+  }));
+};

Evidence

PR Compliance ID 5 requires an automated regression test that fails if the fix is reverted. The test
file implements and tests a local decideEmits() function rather than invoking the actual
socket.emit() decision code in PadMessageHandler, so it is not a true regression test for the
production change.

src/tests/backend-new/specs/new-changes-batch.test.ts[16-36]
src/node/handler/PadMessageHandler.ts[1015-1032]
Best Practice: Repository guidelines

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new regression test does not validate the actual implementation: it defines `decideEmits()` inside the test and asserts on that, so the test can pass even if the real batching logic in `PadMessageHandler.updatePadClients()` is broken/reverted.
## Issue Context
Compliance requires a regression test that would fail without the fix. The production decision currently lives inside `src/node/handler/PadMessageHandler.ts`.
## Fix Focus Areas
- src/tests/backend-new/specs/new-changes-batch.test.ts[16-36]
- src/node/handler/PadMessageHandler.ts[1015-1032]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Rev advanced before send ✓ Resolved 🐞 Bug ≡ Correctness

Description

PadMessageHandler.updatePadClients() advances sessioninfo.rev/time while still collecting revisions,
before any socket.emit() happens; if another updatePadClients() runs in that window (possible due to
un-awaited call sites) it can emit later revisions first, and if emit throws the skipped revisions
will never be retried. The client enforces strict newRev===rev+1 and returns early on mismatch, so
this can silently stop applying updates (pad desync).

Code

src/node/handler/PadMessageHandler.ts[R980-992]

+    // Collect all queued revisions for this socket.
+    const pending: Array<{
+      newRev: number;
+      changeset: string;
+      apool: unknown;
+      author: string;
+      currentTime: number;
+      timeDelta: number;
+    }> = [];
+
   while (sessioninfo.rev < pad.getHeadRevisionNumber()) {
     const r = sessioninfo.rev + 1;
     let revision = revCache[r];

Evidence

The server advances session state before emitting, and there are un-awaited call sites that can
overlap fan-out; the client rejects non-sequential revisions, so missed/out-of-order delivery stops
applying updates.

src/node/handler/PadMessageHandler.ts[950-1036]
src/node/db/API.ts[310-328]
src/static/js/collab_client.ts[191-215]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`updatePadClients()` mutates `sessioninfo.rev`/`sessioninfo.time` while building `pending`, before the corresponding `socket.emit()` is actually performed. This makes `sessioninfo` claim revisions have been delivered when they are only computed locally, which can cause:
- later `updatePadClients()` invocations (that see the advanced `sessioninfo.rev`) to emit subsequent revisions first,
- permanent loss of revisions if `socket.emit()` throws (no retry because `sessioninfo.rev` is already advanced).
## Issue Context
Some call sites invoke `updatePadClients(pad)` without `await`, so overlapping invocations are plausible under load. The client rejects out-of-sequence revisions (`newRev !== rev + 1`) and stops applying.
## Fix Focus Areas
- src/node/handler/PadMessageHandler.ts[950-1036]
- src/node/db/API.ts[310-328]
- src/static/js/collab_client.ts[184-216]
### Suggested fix approach
1. Add per-socket (or per-pad) serialization so only one fan-out per socket runs at a time (e.g., store a `sessioninfo._fanoutPromise` chain and `await` it).
2. During collection, track `nextRev`/`nextTime` in local variables; only commit `sessioninfo.rev/time` **after** a successful emit of the batch (or after each successful per-rev emit in the non-batch path).
3. On emit failure, do not advance `sessioninfo.rev/time` (so the next fan-out can retry), or explicitly force a reconnect/disconnect path if retry is not desired.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---