Repository navigation
feat: announce a bulk realm write once, and let a deploy push without realm events - #6903
Conversation
A /_atomic batch sent one incremental-index-initiation per file it changed. Each one fans out to every subscribed session room, so a catalog deploy that rewrote ~2,000 files queued hundreds of thousands of Matrix sends and pinned Synapse. The batch commit now sends one initiation per commit (naming the first file it changed), and _deleteAllUnlocked one per removal; the splice path no longer sends its own. Add an X-Boxel-Realm-Events: none request header, honored by /_atomic, the +source/binary file POST and the file DELETE. A write carrying it is written and indexed as usual but broadcasts no initiation, update or incremental event, except for an index pass it shares with other writers and was chosen to announce. `boxel realm push --no-realm-events` sends it on every write, so a git-driven deploy doesn't reset open tabs' loaders; they get the new content on their next load. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pass Separate silent writes (a deploy's binary uploads and removals) can be indexed in one coalesced pass. The first writer whose pass announces as it lands speaks for all of them, so a silent writer chosen that way broadcast the pass anyway. A silent writer now joins every pass as a caller that does not announce it: a pass it shares with announcing writers is announced by one of them, and a pass only silent writers share is announced by nobody. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review traced every path that can still broadcast for a write sent with X-Boxel-Realm-Events: none, checked the shared-pass announcer logic, and looked for host or test dependence on per-file initiations. It did not run the realm-server or boxel-cli suites.
Bottom line: I found no correctness bug in the once-per-commit initiation or the shared-pass announcer logic. The one blocking item is the header's stated contract: in production, a silent write still produces prerender_html events, and in rare cases a follow-up incremental event, in every subscribed room.
- Silent writes still broadcast through the worker event bridge. Either suppress these events or narrow the contract. Blocking on the wording. See the comment on
REALM_EVENTS_HEADER. - Should any writer be able to send the header, or only the realm owner or the realm's own server user? Decision, non-blocking. See the comment on
realmEventsSuppressed. - Add a shared-pass test with one silent writer and one announcing writer. Non-blocking. See the comment in
atomic-endpoints-test.ts. - Two comments in
packages/realm-server/tests/card-conditional-write-test.tssay the initiation event is "broadcast per written file": the one aboveindexEventsand the one inside "a refused write enqueues no indexing and broadcasts no event". The initiation is now sent once per commit, naming the first changed file. Please reword both. This PR; non-blocking.
| // A write sent with `X-Boxel-Realm-Events: none` lands and is indexed like any | ||
| // other, but announces nothing to the realm's subscribers: no | ||
| // `incremental-index-initiation`, no file `update`, no `incremental` index | ||
| // event. It is for writers that replace a realm's content wholesale out of | ||
| // band — a deploy pushing a git tree — where every open tab reacting to the | ||
| // push would reset its loader mid-session for code its user did not ask for. | ||
| // Those tabs pick the new content up on their next load instead. | ||
| // | ||
| // Server-side state is unaffected: byte caches, definition caches and peer | ||
| // replicas are invalidated through their own channels, and the index is as | ||
| // current as for any write. Only the Matrix broadcast is withheld. |
There was a problem hiding this comment.
[Claude Code 🤖] In production a silent write still reaches every subscribed room, so "announces nothing to the realm's subscribers" and "only the Matrix broadcast is withheld" are both untrue. The header has no effect on two worker-side broadcasts. Both go through the worker → manager event bridge (reportRealmEvent → realm.broadcastEvent):
prerender_html. The silent write's index pass spawns its prerender_html job as usual. When that job finishes,tasks/prerender-html.tsbroadcasts aprerender_htmlevent that names every URL it rendered. For a deploy, every session room gets one event per render pass, carrying the push's invalidations, and every tab with a live search on the realm re-runs it (resources/search.ts,resources/search-entries.ts). No loader reset happens, but this is the per-room fan-out the header is meant to avoid.- The validation follow-up job. A pass that runs out of peer-commit validation rounds enqueues a follow-up
incremental-indexjob with nocoalescedCallers.tasks/indexer.tsthen announces that job itself as anincrementalevent, which is the event this header exists to withhold. This path is rare.
The realm-server test harness builds its Workers without reportRealmEvent, so the "no realm event was sent" assertions in the new tests can't see either path.
There are two ways out:
- Put the choice on the jobs. For example, add an
announceflag to the spawned prerender_html job and to the follow-up. It is true when any caller of the spawning pass hasannouncesPass, it is OR-merged when prerender_html jobs coalesce, and it takes the writer's own flag inenqueueDeferredPrerenderHtml. - Narrow this comment,
WriteOptions.broadcastRealmEvents, the--no-realm-eventshelp text and the PR description so they say what still goes out.
I'd suppress prerender_html here and document the follow-up.
Class: regression in the new header's own contract. Blocking on the wording at least. The suppression itself can be a follow-up if you'd rather ship the narrower contract.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 0732fb42d1. Incremental index args now carry realmEvents, set from the writes behind the job. It's OR-merged, and absent reads as true. The render job a pass spawns carries a matching announce flag, also OR-merged, and so does a deferred render the realm enqueues itself. A silent pass's render job therefore renders without sending prerender_html. A follow-up pass inherits the flag: it withholds its incremental event but still sends NOTIFY realm_index_updated to the replicas. An announcing publish can't piggyback on a running job that announces nothing, for render jobs and index jobs alike, because a running job's args can't change. The header's doc comment now names prerender_html too.
New coverage: index-commit-validation-test (a silent follow-up pass tells replicas but not hosts), prerender-html-split-test (announce merges OR; an announcing publish won't ride a silent in-flight job) and queue-test (same for index jobs). The realm-server tests still build workers without reportRealmEvent, so the end-to-end assertions don't see worker events. These task-level tests cover that gap.
| function realmEventsSuppressed(request: Request): boolean { | ||
| return ( | ||
| request.headers.get(REALM_EVENTS_HEADER)?.trim().toLowerCase() === 'none' | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] Is it intended that any caller with write can send this header? The host's card saves are unconditional (it sends no If-Match). On a workspace several people edit, that means one collaborator's silent write leaves another's open tab holding the pre-write card. That tab's next save of the card writes the old content back, and nobody is told. The events this header withholds are what would have refreshed that tab. For a deploy pushing to a realm users only read, this is harmless. For a writer in a shared workspace, it is a lost update.
One option: honor the header only for a realm-owner caller or the realm's own server user (the seed-authenticated identity boxel realm push --realm-secret-seed mints), and ignore it otherwise, the way older servers do. If any writer is meant to be able to send it, the header comment should say so.
Class: decision for this PR. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Kept at write, deliberately. The deploy that needs the header pushes as a bot account with read+write on the catalog realm. The realm's only owner is the realm server's own user, so an owner-only check would quietly make the deploy announce everything again. The header can't do anything a writer couldn't already do: it only withholds the announcement of the caller's own write, and a writer can overwrite another tab's card with or without it. In a workspace several people edit, the open tabs' cost is staleness until reload. Their next save already goes through the same last-write-wins path it takes today against any concurrent writer. If we want it narrower later, the natural boundary is a server-minted deploy identity rather than realm-owner. Flagging this as a decision for the human reviewers.
| ); | ||
| }); | ||
|
|
||
| test('per-file source writes and removals sent with X-Boxel-Realm-Events: none send no realm event', async function (assert) { |
There was a problem hiding this comment.
[Claude Code 🤖] Please add a test for one pass shared by a silent write and an announcing write. That is the case the announcesPass: false change exists for, and nothing exercises it. If announcesPass were left true for a silent writer that sits first in coalescedCallers, sharedPassFor would make it the announcer. The announcing writer would stand down, and the silent writer's broadcast would return early, so the announcing writer's tabs would never hear about their own write. Every test in this module is either all-silent or all-announcing, so that regression would pass all of them.
This test also doesn't force its three passes to coalesce, so it doesn't pin the all-silent half either. The description's "whether or not their passes coalesce" isn't something the test controls.
holdIndexingLane and pendingIndexCallers in card-operations-commit-test.ts already build a shared pass deterministically. Two tests on that harness would cover both halves:
- Queue the silent
+sourcePOST first and an announcing write second. Assert exactly oneincrementalevent, carrying the announcing write'sclientRequestIdand naming both writes incoalescedWrites. - Queue two silent writes into one pass. Assert no event.
Class: this PR. Non-blocking, but this is the invariant most likely to break later.
There was a problem hiding this comment.
[Claude Code 🤖] Added in 6dd1794b66: card-operations-commit-test › "a pass shared by a write that asked for no realm events and one that did is announced by the one that did". With the indexing lane held, a silent +source POST joins the pending pass first (the announcer's position), then an ordinary PATCH joins. The test asserts one shared job and exactly one incremental event, under the PATCH's client request id, with coalescedWrites naming both writers. Without announcesPass: false for the silent caller, the silent write would be chosen to announce and the event would be missing.
A write sent with X-Boxel-Realm-Events: none still had its index pass spawn a prerender_html job that announced its rendered URLs to every subscribed room, and a follow-up pass enqueued after validation rounds announced its own incremental event. Incremental index args now carry a realmEvents flag (OR-merged, absent reads as true) set from the writes behind the job; the render job it spawns carries a matching announce flag (also OR-merged), as does a deferred render the realm enqueues itself. A follow-up pass inherits the flag and still notifies peer replicas. An announcing publish no longer piggybacks on a running job that announces nothing, since that job's args can no longer change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mments The announcing writer announces the pass and names the silent write among its coalesced writes. Comments in the conditional-write test described the initiation event as broadcast per written file; it is once per commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
[Claude Code 🤖] Review addressed. In summary:
The description is updated to match. |
The PATCH half targeted a card the fixture didn't have, so it 404'd before queueing and never joined the shared pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ic-write-broadcasts-one-realm-event-per-file-so
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review checked every path a write sent with X-Boxel-Realm-Events: none can still announce through (realm-side broadcasts, the spawned and deferred render jobs, follow-up passes, queue coalescing in both directions), the once-per-commit initiation, and the CLI header plumbing. It did not run the suites.
Bottom line: no correctness issue found in the code. One blocking item on the description: the three "Out of scope" items are deferrals with no linked tickets.
- Each "Out of scope" bullet needs a tracker ticket linked from the description before merge (AGENTS.md, PR Instructions). Two have tickets already (the bounded broadcast queue and the no-op write's
incrementalevent); please link them. The first one, a completion event whose invalidation list exceeds the Matrix event size limit, has none. It is also half of what the originating ticket asks for: an over-limit completion event is rejected bysendEvent, so every subscriber misses that commit'sincrementalevent, andisIndexingstays set until a later event clears it. A silent deploy avoids this, but any other bulk/_atomicwriter (an import, a mass edit) still hits it, and the per-commit initiation now makes those writes look fine until the completion event fails. Please file it and link it, or handle it here. Blocking, AGENTS.md rule.
|
[Claude Code 🤖] @richardhjtan Thanks. The "Out of scope" items now link their tickets in the description:
I've kept the overflow fallback out of this PR. It needs a new event shape and host-side handling (treat it like |
…ic-write-broadcasts-one-realm-event-per-file-so
lukemelia
left a comment
There was a problem hiding this comment.
We should also add skill guidance about when and when not to use this flag.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
[Claude Code 🤖] @lukemelia Added in
There's also a matching entry under Pitfalls, and the generated command list now includes the flag. |
…ic-write-broadcasts-one-realm-event-per-file-so # Conflicts: # packages/boxel-cli/plugin/skills/realm-sync/SKILL.md # packages/boxel-cli/src/commands/realm/push.ts # packages/boxel-cli/src/lib/realm-sync-base.ts # packages/boxel-cli/tests/integration/realm-push.test.ts
Summary
A write to a realm broadcasts realm events to every session room subscribed to that realm. For a world-readable realm, that's every user with a session room. This PR bounds what a bulk write sends, and gives writers that replace a realm's content out of band a way to send nothing at all.
One initiation per commit
#commitBatchUnlockedInnersends a singleincremental-index-initiationper commit, as the first file actually changes. It covers the write, append and removal legs (and the splice path) together._deleteAllUnlockedlikewise sends one per removal. The event keepsupdatedFile, naming the first file the commit changed, so single-file writes produce exactly the event they always did.A subscriber only learns from the initiation that the realm is indexing. The host sets
isIndexingand starts its indexing waiter, and never readsupdatedFile. The completion event says what changed. So a 2,000-file batch costs one initiation per subscribed room rather than 2,000.isIndexingnow goes true once per batch, instead of being retriggered by every file.X-Boxel-Realm-Events: noneA write carrying this header is written, indexed and cache-invalidated exactly as usual, but broadcasts no
incremental-index-initiation, fileupdateorincrementalevent. Nor does the worker announce for it: the render job its index pass spawns sends noprerender_htmlevent, and a follow-up pass enqueued after validation rounds sends noincrementalevent. It is honored by:POST /_atomic+source/ binaryPOSTDELETEThe header exists for writers that replace a realm's content wholesale, like a deploy pushing a git tree. If every open tab reacted to such a push, each would reset its loader mid-session for code its user didn't ask for. Those tabs pick the new content up on their next load instead.
Byte caches, definition caches and peer replicas are invalidated through their own channels (
#notifyFileChange,NOTIFY), which this header leaves alone. Only the Matrix broadcast is withheld.Index passes are coalesced across writers, and one writer announces a shared pass for everyone. A silent writer joins every pass as a caller that does not announce it (
announcesPass: false), so it is never the writer the others stand down for:WriteOptions.broadcastRealmEvents/CommitBatchOptions.broadcastRealmEventscarry the choice through the operation core.On the worker side,
IncrementalArgs.realmEventsandPrerenderHtmlArgs.announcecarry it into the jobs. Both are OR-merged when jobs coalesce, so one announcing write makes the merged job announce, and both read as true when absent (a job enqueued by an older realm server). An announcing publish never piggybacks on a running job that announces nothing, since a running job's args can no longer change. A silent follow-up pass still sendsNOTIFY realm_index_updated, so peer replicas drop their caches.Any caller with
writecan send the header. The deploy that uses it pushes as a non-owner writer. The header withholds only the announcement of the caller's own write, which grants nothing a writer couldn't already do.boxel realm push --no-realm-eventsThe push sends the header on every write it makes: the
/_atomicbatch, each binary upload and each--deleteremoval. A deploy workflow can then push a realm without announcing it. The catalog's deploy workflows will pass the flag once a CLI with it is published. Older realm servers ignore the header.Out of scope
incrementalevent a write that changes nothing still broadcasts: CS-14226 (Announce a write that changes nothing only to the writer that sent it #6905).Test plan
atomic-endpoints-test›realm events(new):/_atomicproducesinitiation, update, incremental, with the initiation naming the first file and the completion naming all three cards.X-Boxel-Realm-Events: noneproduces no event and the cards are indexed. A byte-identical repeat produces none either.POST, a binaryPOSTand aDELETE, each with the header, produce no event, whether or not their passes coalesce.deleteAllof three files produces one initiation.card-operations-commit-test(new): a pass shared by a silent+sourcePOST (joined first) and an ordinary PATCH is announced once, by the PATCH, withcoalescedWritesnaming both.index-commit-validation-test(new): a silent follow-up pass sends no realm event but still notifies the replicas.prerender-html-split-test/queue-test(new):announce/realmEventsmerge as OR, and an announcing publish doesn't join a running job that announces nothing.realm-push.test.ts(new): a plain push marks none of its writes.--no-realm-events --force --deletemarks the/_atomicPOST, the binary POST and the DELETE withX-Boxel-Realm-Events: none, and the changes land.atomic-endpoints-test(23 passing),card-conditional-write-testandcard-operations-commit-test. CI covers the rest of the realm-server shards, includingcard-source-endpoints-testandrealm-endpoints-test, which assert single-file initiation events.🤖 Generated with Claude Code