Encode a broadcast once, not once per connection (#288)

[scgopi]

Stacked on #293; rebases onto main once that lands.

The defect

GraphStore.notifyClients looped over connections calling send(_:to:), and send ran JSONEncoder().encode(event) — so a graph change cost one full encode of the snapshot per connection. With C clients attached that was C encodes of the same bytes on every change, presence tick included: the CPU amplifier the #288 root-cause report named beside the blocking write that #291 removed.

The change

The encode is hoisted out of the loop. notifyClients encodes the snapshot once into an EncodedEvent — the bytes plus the per-graph superseding key #291 introduced, everything about a frame that does not depend on who receives it — and hands that to every connection through deliver(_:to:). send(_:to:) keeps its shape for the unicast callers (the joining snapshot, a refusal) and now just encodes and delivers. announceError gets the same treatment. The one behaviour callers rely on — a connection whose channel is gone is dropped by the broadcast that finds it so — lives in deliver, unchanged.

C encodes → 1 per broadcast; nothing else moves.

Verification

Gate: full Xcode suite (1616 tests in 168 suites, 0 failures), swiftlint 0 errors, swift-format clean, graphcoded and graphcode-cli schemes build; SwiftPM swift build and .build/debug/graphcode pass locally.

Encoding is not observable from outside the actor, so BroadcastEncodingTests pins what the refactor must keep: two connections receive byte-identical frames for one change, a connection whose channel was closed is dropped on the broadcast that finds it, and the live one still hears the change. #289's diagnostics (next PR) record the encode duration once per broadcast, which is where the count becomes visible.

Refs #288

🤖 Generated with Claude Code

https://claude.ai/code/session_01DeGL2CxuGmq16RSZpJYm2N

[scgopi]

Independent review of #296 at be549b0e — I would merge this

Reviewed in my own worktree (review/296-297-probes), not the author's. This is a small, honest refactor and I could not break it.

The four #291 invariants — all four still hold

#291 established these in exactly the code #296 rewrites, and each was a real bug a reviewer caught. Verified by reading the post-#296 tree, not the diff:

#	Invariant	Where it lives now	Status
1	supersedingKey is per graph, never a constant	GraphStore.encode, "graphChanged:\(changed.id)" — moved out of send into EncodedEvent, still derived from the event's own graph	✅ intact
2	OutboundChannel writes MSG_DONTWAIT + bounded poll, not a blocking write	OutboundChannel.swift:286 / :302, untouched by this PR	✅ intact
3	Valve measures pendingBytes - data.count	OutboundChannel.swift:142, untouched	✅ intact
4	OutboundChannels.send refuses an unregistered fd	OutboundChannel.swift:384 — guard let channel else { return false }, no lazy creation	✅ intact

Invariant 1 is the one this PR could plausibly have broken, and it doesn't. The key is computed per encode, and every notifyClients encode is for exactly one store's own graph, so one key per broadcast is the same key send would have computed per connection. The key travels with the frame in EncodedEvent rather than being computed once and reused across graphs — which is the distinction that matters.

The behaviour callers depend on

deliver keeps the else { connections.removeValue(...) } on a false return, and it is the only path to OutboundChannels.send, so both the broadcast and unicast shapes still forget a dead client. ✅

announceError is equivalent: .errorOccurred carries supersedingKey == nil before and after, and onAnnounceError? still fires whether or not the encode succeeded.

The for id in connections.keys loop mutating connections from inside deliver is safe for the same reason it was before — the Keys view triggers COW on the first removal and the iteration finishes over the original.

Verification I ran myself

• Full Xcode gate: 1619 tests / 169 suites / 0 failures, run by me to completion. To be precise about what that measures: it ran on 4392e12, the pre-rebase tip of Ship the presence tick as the loops it moved, not the whole graph (#288) #297 (which contains Encode a broadcast once, not once per connection (#288) #296). I have not run a full gate on be549b0e itself. Linux CI is green on be549b0e.
• Rebase verified byte-identical. git diff origin/main..be549b0e and git diff ac19002..dd3e723 differ only in hunk headers. The rebase onto post-Serve the Mailroom from a bounded mailbox request, not every broadcast (#288) #293 main dropped nothing from either side, which was the specific risk — both branches edit GraphStore.send and notifyClients.

One thing worth fixing (not a blocker)

notifyClients now encodes even when nobody is connected. Before, send checked connections[connectionID] first and only then encoded, so a zero-connection store did zero encodes — the loop body simply never ran. Now the encode is hoisted above the loop and is unconditional.

That matters because the daemon deliberately runs with no client attached: pollPresence guards !connections.isEmpty for exactly this reason ("with no client attached there is no surface to show it on"), but broadcast() has no such guard, and predicate polling, session lifecycle and time-based triggers all reach it. On this repo's own graph the wasted encode is a 63,482 B snapshot per graph change, going nowhere.

It's a pure regression in the one dimension this PR is about, and a one-liner:

private func notifyClients() {
  guard !connections.isEmpty else { return }
  ...
}

Nit

send(_:to:) now encodes before checking that the connection still exists, so a unicast to a departed client pays for an encode it throws away. Trivial next to the above, and fixed by the same reordering if you'd rather do it once.

---

Verdict: approve. Fix the zero-connection encode if you agree it's a regression; I don't think it needs another round of review either way.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QhMkgJFi1Nhnh9UWwkSxDm