Repository navigation
Conversation
When every file in a commit is byte-identical to what is stored, the commit queues no index pass, yet it still broadcast an `incremental` event with no invalidations to every session room subscribed to the realm — every user's room, for a public realm. Bulk writers that resend unchanged content (the nightly OpenRouter sync, the skills and homepage syncs) paid one fan-out per request for nothing. Such a commit now announces nothing when the request named no client request id, since nobody can be waiting for that event. When it did name one, the event goes only to the writer's own session room: the host's AI-assistant waiter keys its wait on the event carrying its request id, and only the writer's client can be holding that wait. A module flush that ran ahead of byte-identical instances, or a shared pass this writer was chosen to announce, is still announced to every subscriber. RealmAdapter.broadcastRealmEvent takes an optional delivery argument whose `recipients` narrows the send to those matrix users' session rooms. 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 looked at four things: who can be waiting on a no-op write's index event, how passOwed interacts with the announcedByPeer stand-down in broadcastIncrementalInvalidationEvent, what initiatingUser holds on each front door into #commitBatchUnlockedInner, and every implementation of broadcastRealmEvent. The waiters checked were the host tool waiter, the store, file resources, base cards, boxel-cli, ai-bot and the test helpers. The realm-server and host suites were not run.
No blocking issues. Any pass behind the event still reaches every subscriber. The only listener keyed on a request id, ToolService.trackAiAssistantCardRequest, runs in the writer's own session, so the writer-only send reaches it.
- Name the writer the null-
initiatingUserfallback is for, or drop the fallback. See the comment on therecipientsspread inrealm.ts. Non-blocking question. - Follow-up: filter the recipients inside the session-room query instead of after it. See
node-realm.ts. Non-blocking.
Adjacent, out of scope: fetchRealmSessionRooms matches the * row and per-user rows but not the users row that effectiveRealmPermissions honors. A user whose access comes only from users therefore gets no realm events at all, including the writer-only event here. The RealmEventDelivery comment ("every user who can read the realm") overstates delivery for the same reason. This is not this PR's bug. It's flagged for whoever touches that query next.
| ...(!passOwed && initiatingUser | ||
| ? { recipients: [initiatingUser] } | ||
| : {}), |
There was a problem hiding this comment.
[Claude Code 🤖] Which writer is the null-initiatingUser fallback for? When initiatingUser is null, this still sends the full fan-out to every subscriber. The comment above justifies that as "the only way to reach" a writer the realm cannot identify. I can't find such a writer that could be holding a wait:
- A signed-in host always carries
authenticatedUser. - An anonymous writer admitted by a policy grant does not arrive here as null. It arrives as the grant's acting user (
opts.actor ?? opts.actingUser?.()incard-operations/coordinator.ts), so it gets the narrowed send to that account's room. The "cannot identify" wording doesn't describe that case. - A caller with no matrix session has no session room, so neither kind of delivery reaches it.
- The remaining null cases are realm-internal writes.
If no such caller exists, gating on passOwed || (clientRequestId && initiatingUser) would keep an anonymous re-send of unchanged content out of every subscriber's room. That is the cost this change sets out to remove. If one does exist, please name it in the comment so the next reader doesn't remove the fallback.
Scope: this PR's own code. It's a question, not a demonstrated defect. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Kept the fallback, and named the writer it reaches in 39a8edf669. A world-writable realm admits a write that carries no Authorization header as anonymous (createRequestContext sets anonymous and leaves authenticatedUser unset), so initiatingUser is null. The host that sent it is still signed in to Matrix and can be holding an AI-assistant waiter keyed on its request id. With passOwed || (clientRequestId && initiatingUser) that waiter would sit until its timeout, because the realm has no room to narrow to. So that one case keeps the full fan-out. It is narrow (an anonymous write, to a world-writable realm, that changed nothing and named a request id), and none of the bulk writers take it.
| if (opts?.recipients) { | ||
| let recipients = new Set(opts.recipients); | ||
| dmRooms = Object.fromEntries( | ||
| Object.entries(dmRooms).filter(([userId]) => recipients.has(userId)), | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] The narrowed send still loads every subscriber's session room and filters them in memory. On a public realm, fetchRealmSessionRooms returns one row per registered user, so a writer-only event pays the same read as the fan-out did and then sends a single event. Passing the recipients into the query (AND u.matrix_user_id = ANY(...) in db-queries/session-room-queries.ts) would make it a lookup of the named users only.
Follow-up, not this PR. The bulk writers behind this change name no request id and never reach this path. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Agreed that this is worth doing, and leaving it as a follow-up. NodeAdapter.broadcastRealmEvent and its session-room lookup are being rewritten into a paced per-process dispatcher in a separate open change. Pushing recipients into the fetchRealmSessionRooms query belongs in that new lookup, rather than in the loop it replaces. The narrowed path only runs for a no-op write that names a request id, which in practice is a host session, so it stays rare until then.
The fallback that sends a no-op write's event to every subscriber exists for a request admitted without a user: an anonymous write to a world-writable realm has no session room to narrow to. Say so, and describe default delivery as what fetchRealmSessionRooms returns rather than as every user who can read the realm. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t-per-file-so' of https://github.com/cardstack/boxel into cs-14226-a-realm-write-that-changes-nothing-still-broadcasts-an
|
[Claude Code 🤖] Addressed the review in |
Host Test Results 1 files ± 0 1 suites ±0 16m 9s ⏱️ + 7m 53s Results for commit 067e075. ± Comparison against earlier commit 69f7643. Realm Server Test Results 1 files ±0 361 suites ±0 1h 49m 41s ⏱️ - 8m 19s Results for commit 067e075. ± Comparison against earlier commit 69f7643. |
…t-per-file-so' of https://github.com/cardstack/boxel into cs-14226-a-realm-write-that-changes-nothing-still-broadcasts-an
# Conflicts: # packages/realm-server/node-realm.ts # packages/realm-server/tests/node-realm-test.ts
|
[Claude Code 🤖] Merged the updated base branch (now including #6899's realm event queue) in ba6fdd3. Two files conflicted:
Verified locally: |
# Conflicts: # packages/realm-server/tests/atomic-endpoints-test.ts
|
[Claude Code 🤖] The base branch has merged, so this PR now targets
Verified locally: |
This PR's base is the branch that announces a bulk write once and adds the
X-Boxel-Realm-Events: noneopt-out. It reworks the same batch-commit broadcast code, so review it first. This PR is the single commit on top of it.What this changes
A commit that finds every file already holding the bytes it staged writes nothing and queues no index pass. Even so, it broadcast an
incrementalindex event with no invalidations to every session room subscribed to the realm. For a public realm that means every registered user's room. Bulk writers resend unchanged content: the OpenRouter models sync sends ~600 operations in batches of 20, and the skills and homepage syncs upload every file. Each of their requests cost a full fan-out that told nobody anything.The no-op branch of
#commitBatchUnlockedInnernow decides who the event is owed to:ToolService.trackAiAssistantCardRequest, awaited by the correctness check). A patch that changes nothing would otherwise stall that check until it times out. Only the writer's client can be holding such a wait, so the event goes to that one room. A writer the realm cannot identify still gets the event as every subscriber does.A no-op commit already sent no
incremental-index-initiationand no fileupdateevent. So the host's indexing waiter, which opens on an initiation and closes on an index event, stays balanced.Delivery to named recipients
RealmAdapter.broadcastRealmEventtakes an optionalRealmEventDeliverywhoserecipientsnarrows the send to those matrix users' session rooms.NodeAdapterfilters the realm's session rooms by matrix user id. The host test adapter has no per-user rooms and ignores it.Test plan
atomic-endpoints-test, a write that changes nothing announces its index event only to its own writer:/_atomicupdate with no request id broadcasts nothing;incrementalevent goes out, carrying that id and addressed to the writer only;Reverting the gate makes this test fail on its first assertion: the no-op write broadcasts an
indexevent.node-realm-test, delivers only to the named recipients when the broadcast names any: with no recipients every subscriber's room is sent to, and with recipients only theirs. A recipient with no session room receives nothing.🤖 Generated with Claude Code