Repository navigation
Conversation
A realm event used to be posted into the session room of every user with a grant on the realm, so a 30-member realm cost Synapse 30 stored events per notice. A private realm (no '*' read grant) now gets one invite-only Matrix room, whose members follow the realm's named grants, and each event is sent into it once. - realm_event_rooms table; queries for the room and the named grantees - MatrixClient: create the room (invite-only, history from invite, only the creator may send/invite/kick), list members, invite, kick, leave - dispatcher: schedule() runs a job on a realm's line, in order with its events, never merged or dropped - realm-event-rooms.ts: ensure / sync / retire the room as line jobs - NodeAdapter routes a private realm's event to its room when it has one - POST /_realm-event-rooms answers a named grantee, inviting them first - membership syncs after PATCH /_permissions, the server grant upsert, and on realm mount; realm delete retires the room - host joins the room on first subscription (ported from the closed shared public-room change), with its own sliding-sync list Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The room's creator in the test harness isn't the user the tests assumed, so reading state as that user got a 403. A grantee joining proves the invite, and as a member it can read the room's state. Also name both entry kinds in the dispatcher's prepare-failure log. 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe3052ef02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${this.token}`, | ||
| }, | ||
| body: JSON.stringify({ realms }), |
There was a problem hiding this comment.
When more than 100 realms are pending—particularly after reaskRealmEventRooms() synchronously requeues every subscribed realm—this sends all of them in one request, while the new endpoint rejects any request over 100 with HTTP 400. The catch block then restores the same full set, so every retry is identically oversized and the host never joins any newly needed event rooms, causing private-realm notifications to be missed. Split realms into batches no larger than MAX_REALMS_PER_REQUEST.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 8e2b3e8's parent efa8390. The host asks /_realm-event-rooms about at most 100 realms per request (REALM_EVENT_ROOMS_PER_REQUEST). The rest stay pending and the task runs again once a request succeeds, so a re-ask of many realms drains in batches instead of repeating one oversized request.
| let stored = await insertRealmEventRoom( | ||
| deps.dbAdapter, | ||
| deps.realmURL, | ||
| created, | ||
| ); |
There was a problem hiding this comment.
When multiple realm-server processes serve the realm, inserting the room mapping here makes it visible before membershipSends() has sent the initial invitations. Another process can immediately have prepareRoomSends() switch broadcasts from session rooms to this room; because its history visibility is invited, grantees cannot receive or later read events sent before their invite. A save or indexing burst during room creation can therefore be lost even though the endpoint eventually invites the user and succeeds; keep the mapping unavailable to broadcasters until initial membership is established.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in efa8390. A new room is stored as the realm's only after its first invites have gone out (createRoom in realm-event-rooms.ts), so no process can route an event to it before its members can see it. Creation is serialised per realm within a process, so a second ask meanwhile can't make another room. A lost race across processes retires the unused room and invites the grantees to the winner. The membership test checks that the row is still absent at every invite.
| // The realm's event room holds the users its grants name, so a grant | ||
| // that names someone new invites them and a revoked one removes them. | ||
| // Not awaited: the invites and kicks are paced behind the realm's events. | ||
| void this.syncRealmEventRoom(); |
There was a problem hiding this comment.
When a grant is revoked in a multi-process deployment, this fire-and-forget sync only queues the kick on the current process's in-memory dispatcher. Until that kick reaches the head of the line—which can be delayed by a realm backlog or Matrix back-pressure—another web or worker process continues reading the stored room ID and can send new realm notices to the room, so the revoked, still-joined user receives post-revocation file URLs and invalidation data. Mark the room unavailable for sends or otherwise coordinate membership changes across processes before treating the revocation as complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in efa8390. A revocation retires the room before PATCH /_permissions answers: retireRealmEventRoomRow moves the row to retired_realm_event_rooms in one statement, so from then on no process reads a room for the realm and its events go to session rooms, which re-read the grants for every event. Removing the room's members happens afterwards, in the background, and no longer gates whether the revoked user hears anything. The endpoint test asserts the row is gone when the revocation returns.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review went after the membership invariants (who can share or see the room under ADR 011), failure paths in the dispatcher jobs, every path that writes realm_user_permissions, and whether the tests reach the wiring. I didn't run the realm-server or host suites, and I didn't look at the load harness changes.
Two blocking issues, both under ADR 011. A revoked member stays visible in the room's state to users granted later, and a delete whose kicks fail leaves its former grantees in a room no one can administer. The rest of the design holds up: the send path, the creation race, the paced jobs, and the '*' handling. Every path that writes grants is covered by a sync, a mount, or the ensure-on-ask. Create and publish start a realm with no room, and unpublish touches only world-readable realms.
- Rotate the room on revocation, or change ADR 011 §1.2. See the kick loop in
realm-event-rooms.ts(membershipSends). - Read memberships before forgetting the room on delete, and don't leave when a kick failed. See
retireRealmEventRoom. - Answer 503, not a 200 without the realm, when back-pressure exhausts the ensure job. See
ensureRealmEventRoom. - Add tests that go through the mount, upsert and delete call sites. See
Realm.startinruntime-common/realm.ts.
Out of scope for this PR: on a '*' change, patchRealmPermissions now calls notifyRealmsListUpdated for every named user, one awaited send at a time inside the request, outside the paced dispatcher. That's fine at classroom size. A realm with hundreds of grantees would want those sends on the dispatcher too.
| for (let user of memberships.keys()) { | ||
| if (isMember(user) && !wanted.has(user)) { | ||
| sends.push({ | ||
| roomId, | ||
| send: async () => { | ||
| await deps.matrixClient.kickUser( | ||
| roomId, | ||
| user, | ||
| 'No longer has a grant on this realm', | ||
| ); | ||
| }, | ||
| onFailure: membershipFailure(deps, roomId, `kick ${user}`), | ||
| }); |
There was a problem hiding this comment.
[Claude Code 🤖] A kick takes a revoked user out of the room's members, but not out of its state. Their m.room.member event (now leave) stays in current state, and anyone who joins later can read it. GET /rooms/{id}/members returns every membership in current state unless the caller filters it. history_visibility: invited limits the timeline, not current state. So a user granted access after R was revoked learns R's Matrix id, though the two never held grants on the realm at the same time. The same goes for a user who was invited and then revoked before ever joining. ADR 011 §1.1 forbids this: it names room membership and room state. §1.2 allows only users who each hold a grant that names them.
Two ways out:
- Rotate the room when a sync would kick anyone. Create a fresh room, invite the current grantees, store it, retire the old one, and send
realms-list-updatedto the grantees so their hosts ask again and join. That costs a room and N invites per revocation, and revocations are rare next to notices. - Change ADR 011 §1.2 so a realm's grantees may see users who held a grant on it before them, and say so in the ADR.
I'd take the first. Either way, pin it with a test: revoke one user, grant a new one, and check that the newcomer can't see the revoked user's id in /members.
Regression, blocking (ADR 011 §1.1, in new code).
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in efa8390, by rotating. A member is never kicked from the realm's live room. A revocation (or a sync that finds a member no grant names) retires the room at once, the remaining grantees get realms-list-updated, and their hosts' next ask makes a fresh room that holds only current grantees. The endpoint test pins it: grant a user, revoke them, ask again, and the revoked user's id is nowhere in the new room's /members.
| await clearRealmEventRoom(deps.dbAdapter, deps.realmURL, roomId); | ||
| retired = roomId; | ||
| await loginIfNeeded(deps.matrixClient); | ||
| return await membershipSends(deps, roomId, []); |
There was a problem hiding this comment.
[Claude Code 🤖] If anything after clearRealmEventRoom fails, the room is left with all its members, no one who can administer it, and no row pointing at it. retired is set before membershipSends reads the members. If that read rejects with back-pressure, the dispatcher prepares the job again, but the row is gone, so the retry returns []. If it rejects with any other error, the dispatcher skips the job. Either way the leave job below still runs, and the realm server leaves a room whose members it never kicked. Nothing can repair that afterwards. Only the creator has the power to kick, and it has left. No other record of the room exists. So the deleted realm's former grantees stay in the room together for good, which ADR 011 §1.5 forbids once their grants are gone. A single kick that fails (its onFailure only logs) is followed by the same leave, with the same result for that member.
Suggested order: read the memberships before clearing the row, and set retired only once that read succeeds. Have the kicks' onFailure record the failure, and skip the leave when any kick failed, so the room stays one the realm server can still administer. Better still, keep a record a later pass can retry, for example a retired flag on the row, instead of deleting the row first. A fake client whose getRoomMemberships throws once would pin this.
Regression, blocking (ADR 011 §1.5).
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in efa8390. A retired room keeps its record (retired_realm_event_rooms) until every member is kicked and the realm server has left. The members are read before anything else; if that read fails, or any kick fails, the server stays in the room and the record stays. The realm's next sync retries it, and a sweep at server start (emptyAllRetiredRealmEventRooms) covers deleted realms, which never mount again. Two membership tests use a fake client whose kick, and whose member read, fail once, and check that the next sync finishes the job.
| let room = stored ?? (await createRoom(deps)); | ||
| let sends = await membershipSends(deps, room, grantees); | ||
| roomId = room; | ||
| return sends; | ||
| } catch (e) { | ||
| if (isBackPressure(e)) { | ||
| throw e; | ||
| } | ||
| failure = e; | ||
| return []; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] When back-pressure outlasts the dispatcher's maxAttempts, this job ends with neither roomId nor failure set. The dispatcher logs, skips the job and resolves its waiter, so ensureRealmEventRoom returns undefined and the handler leaves the realm out of a 200. The host reads that as "this realm has no room". It retries only on a non-OK status, and realmEventRoomsRequested stops it from asking again until a realms-list-updated. If the room exists, the realm's events go there, and a host that wasn't already joined gets nothing for that realm until it reloads. This happens when Synapse stays overloaded long enough, which is the load this PR is meant to survive.
Fix: once the realm has passed the grantee check, treat a job that resolves without roomId as a failure. For example, record the back-pressure error before rethrowing it, and throw it after schedule resolves. The handler then answers 503, and the host backs off and asks again.
Related: when membershipSends forgets a stored room the server is no longer in, roomId = room still returns that dead room. The host's join fails and its retry recovers, but returning a failure here would save the round trip.
Regression, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in efa8390. Once the realm passes the grantee check, the lookup rejects when its job didn't run to the end, back-pressure exhaustion included, so the endpoint answers 503 and the host backs off and retries. A stored room the server is no longer in is forgotten and replaced in the same ask, so it's never returned. Both cases have a membership test.
| // Repairs a grant change no process applied to the realm's event room, | ||
| // such as one made while nothing served the realm. Not awaited: it is | ||
| // paced with the realm's events and never rejects. | ||
| void this.syncRealmEventRoom(); |
There was a problem hiding this comment.
[Claude Code 🤖] No test reaches this call. None reaches the syncRealmEventRoom call in handle-upsert-realm-user-permission.ts or the retireRealmEventRoom call in handle-delete-realm.ts either, so deleting any of the three leaves the suite green. "syncing repairs a grant change made while nothing applied it" calls testRealm.syncRealmEventRoom() directly, and the retire test calls retireRealmEventRoom directly. Repair after a restart is what this line exists for, so it deserves a test that goes through it: write a grant to the table, mount the realm (or restart the test realm), and wait for the invite. Add one through the server-level grant upsert endpoint, and one through DELETE of a realm that has a room.
Test gap in this PR's own code, non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in efa8390 and 8e2b3e8. delete-realm-test stores a room the server isn't in for a realm URL, creates the realm there, and waits for the mount's sync to forget it, then deletes the realm and checks the room was retired before the delete answered. maintenance-endpoints-test does the same check through the server-level grant upsert. That endpoint never revokes (it refuses read=false&write=false), so 8e2b3e8 dropped the revocation branch I'd added there.
A kick leaves the revoked user's membership in the room's state, where a user granted later could read it, and a kick queued behind the realm's events left the revoked user receiving them meanwhile. A revocation now retires the room at once (the row moves to retired_realm_event_rooms in one statement, so every process falls back to session rooms), the remaining grantees are told to ask again, and the next ask makes a fresh room. A retired room is emptied (every member kicked) and left only when every removal succeeded; its record stays until then and is retried by the realm's next sync, and by a sweep at server start for deleted realms. A new room is stored only after its first invites go out, creation is serialized per realm in-process, and a lost creation race retires the unused room and invites the grantees to the winner. ensureRealmEventRoom rejects when its job couldn't run (so the endpoint answers 503), and makes a fresh room when the stored one is dead. The host asks about at most 100 realms per request. Tests go through the mount, grant upsert and delete wiring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n branch the grant upsert can't reach The grant upsert endpoint refuses read=false&write=false, so it never revokes; it only syncs. A failed creation no longer leaves an unhandled rejection from the lock's cleanup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
[Claude Code 🤖] Thanks. Every inline finding is answered in its thread, with the fix commit, and all four are fixed in efa8390 and 8e2b3e8. The description now describes the rotation design. On the out-of-scope note: you're right that the |
|
[Claude Code 🤖] This replies to the review summary:
Both are fixed, in efa8390 and 8e2b3e8:
The two non-blocking findings are fixed in the same commits, and their threads have the details. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lukemelia
left a comment
lukemelia
left a comment
There was a problem hiding this comment.
Assuming ADR 011 is approved and merged, I am OK with this
|
[Claude Code 🤖] Thanks, @lukemelia. Agreed, this depends on ADR 011, and the description opens with Merges after: #6921 so it doesn't land before the ADR is accepted. |
Merges after: #6921 (ADR 011).
When a card in a realm changes, the realm server tells every open tab that can see the realm, so the tab can refresh what it shows. That notice is a Matrix event of type
app.boxel.realm-event("indexing started", "these files changed", "indexing finished"). Each user has a private DM with the realm server, their session room, and each notice is posted into the session room of every user with a grant on the realm. One save usually produces two or three notices. So a realm with N members costs Synapse 2–3 × N stored events per save, whether or not those members have the realm open.That multiplier is what overloaded production Synapse: one 30-member classroom realm, during an ordinary burst of edits, produced about 93 stored events a second, close to what Synapse can absorb. Sign-in then queued behind it.
This PR gives each private realm one Matrix room for its notices. A private realm is one with no
'*'read grant. The room's members are exactly the users the realm's grants name, so each notice is posted once. For that realm, 1 stored event instead of 30.Why a shared room is allowed here
ADR 011 says two end users may share a room only when they may see each other's Matrix ids (§1.5), that users who each hold a grant naming them on the same realm may (§1.2), and that no one else may, through room membership or room state (§1.1). A Matrix room shows its member list to every member, so the room must hold no one the grants don't name. That rules out a world-readable realm, whose readers aren't named. So:
'*'grant names no one, so it adds no one to a room, and a realm that gains'*'stops sending to its room.@user:serverids only. The'*'grant, theuserssubject (any signed-in user) and a bare username name no one. Access that comes only from a realm policy isn't a row inrealm_user_permissions, so it doesn't count either.join_rule: invite, guest access forbidden, not in the room directory). No one can join it without an invite from the realm server.history_visibility: invited). A new member never sees notices from before they were invited. They do see one sent between their invite and their join, which covers a notice that lands while their tab is still joining. A removed member keeps the notices they already received. Those hold card URLs and invalidation lists, never card content, and the room's retention policy bounds them.How membership follows the grants
packages/realm-server/realm-event-rooms.tsholds the logic. The rule it keeps is that a room's state never shows anyone a user they don't share a grant with. Matrix keeps a kicked user's membership event (nowleave) in the room's state, so a revoked user is never kicked out of the realm's live room. Grantees are users with their own read or write grant,@user:serverids only (fetchRealmNamedGrantees).PATCH /_permissionsand the server-level grant upsert, when a host asks for the room, and when the realm mounts.retired_realm_event_roomsin one statement before the revoking request answers. From then on no process sends the realm's events there, and they go to session rooms, which re-read the grants for every event. The remaining grantees getrealms-list-updated, so their hosts ask again, and that ask makes a fresh room holding only current grantees.All of the Matrix work runs through the paced realm event dispatcher. That's one line per realm, sending at a bounded rate and pausing when Synapse pushes back. It gains
schedule(job), which queues work other than an event on a realm's line. A job takes its turn between the events queued before and after it, never merges with an event, and is never dropped when the queue is over its byte bound. So a grant change on a large realm is paced like any burst of notices, and can't flood Synapse.Sending
NodeAdapter.prepareRoomSendsdecides where a notice goes:recipients, e.g. a write that changed nothing goes back to its writer only) → those users' session rooms, as before.The host
The host asks
POST /_realm-event-rooms({ realms: string[] }→{ rooms: { [realm]: roomId } }) about each realm as it first subscribes to it, and joins the rooms it gets back. The endpoint answers only for private realms whose grants name the signed-in user. It checks that in the database before mounting anything, so a stranger can't make the server mount a realm. A request names at most 100 realms and is worked through four at a time. A failed lookup answers 503 with the rooms it did find, and the host retries the rest with backoff. When a grant change reaches the host asrealms-list-updated, it asks again about every realm it has open. A change to the'*'grant sends that to every user the realm names, since it moves where their notices go.The rooms have their own type,
app.boxel.realm-events. They get their own sliding-sync list, with the session rooms' timeline limit, because they get the same bursts of notices. They are kept out of the AI room list. Events from them are relayed exactly as session-room realm events are.Rollout
Nothing sends to both places. Until a host running this code asks for a realm's room, that realm's notices keep going to session rooms, so tabs that are already open keep working. Once one asks, the realm's notices go to the room, and a tab loaded before the deploy stops getting them until it reloads.
Open points for review
Tests
realm-event-rooms-test, membership (fake matrix client, real DB):'*', not a bare username);realm-event-rooms-test, endpoint (real Synapse):invitedhistory;PATCH /_permissionsgrant invites;delete-realm-test: mounting a realm checks its stored room, and deleting the realm retires it before answering.maintenance-endpoints-test: the server-level grant upsert checks the realm's room.node-realm-test:realm-event-dispatcher-test: a scheduled job runs between the events around it without letting them merge across it, and is never the entry the byte bound drops.permissions-test: changing the'*'grant pushesrealms-list-updatedto a user the realm already named.setupMatrixRoomasks for the realm's room and joins it the way a host does, and reads both rooms. So the existing tests that wait for notices on private test realms run through the new room.message-service-test: subscribing to a private realm joins its room, and a notice sent into the room reaches the realm's subscriber. The host asks about at most 100 realms per request.🤖 Generated with Claude Code