WPB-28483: remove serial per-user round trips from MLS commit-bundle path - #5528
Open
blackheaven wants to merge 5 commits into
Open
WPB-28483: remove serial per-user round trips from MLS commit-bundle path#5528blackheaven wants to merge 5 commits into
blackheaven wants to merge 5 commits into
Conversation
zebot
added
the
ok-to-test
…path POST /v17/mls/commit-bundles spent ~800ms on sequential I/O for a 7-user internal_add join. Remove the avoidable round trips without changing protocol semantics: - getCommitData fetches all pending proposals once; commit proposal refs are dereferenced from that list (derefOrCheckProposalFrom) instead of one CQL point-read per ref, and checkReferences reuses the same list instead of re-scanning the partition. - incrementEpochNoRead skips the full conversation re-read on the internal-commit path, where the result is discarded. - brig client-data lookups in checkClients fan out concurrently (getClientData is total; validation stays serial so Error-effect failures still abort the whole commit exactly as before). - addMLSClients logged batches run concurrently (disjoint rows, children kept Error-free so failures propagate as IO exceptions). - Local welcome pushes are fire-and-forget via pushNotificationAsync; delivery is already asynchronous downstream of gundeck.
blackheaven
force-pushed
the
gdifolco/WPB-28483-mls-commit-bundle
branch
from
September 8, 2026 17:26
fafca76 to
eb2d09d
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent paths currently discard sequenceConcurrently failure signals (outer Nothing), which can silently ignore exceptions and lead to accepting commits with incomplete validation or partial persistence.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Reduces end-to-end latency in the MLS commit-bundle processing path by removing per-ref and per-user serial round trips and by making downstream fan-out work asynchronous/concurrent where possible.
Changes:
- Prefetch pending proposals once per commit-bundle and dereference commit proposal refs from that in-memory list.
- Introduce concurrent fan-out for client data fetching and client-store writes; add an epoch bump variant that avoids re-reading the conversation.
- Make local welcome notification delivery fire-and-forget via
pushNotificationAsync(no longer blocks commit-bundle response).
File summaries
| File | Description |
|---|---|
| libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/Welcome.hs | Switch local welcome delivery to async push to avoid blocking commit-bundle response. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/Proposal.hs | Replace per-ref store reads with deref-from-prefetched pending proposals list. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/Message.hs | Thread Async through commit-bundle processing and pass prefetched proposals into internal commit processing. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/Commit/InternalCommit.hs | Run client-store additions concurrently and avoid re-reading conversation after epoch bump. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/Commit/Core.hs | Prefetch pending proposals once; add incrementEpochNoRead helper. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/MLS/CheckClients.hs | Fetch client data concurrently (requires Async). |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/Interpreter.hs | Add Async capability to the conversation subsystem interpreter stack. |
| libs/wire-subsystems/src/Wire/ConversationSubsystem/Federation.hs | Adjust effect constraints for welcome handling after async push change. |
| changelog.d/5-internal/WPB-28483 | Document reduced sequential I/O and non-blocking welcome pushes. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Under galley's production stack, a spawned child whose Error-effect throw has no interpreter inside the asyncToIOFinal boundary collapses to Nothing instead of propagating. Both concurrent fan-outs now treat that collapse as a hard failure instead of conflating it with legitimate results: - CheckClients.checkClients: a collapsed brig client-data fetch aborts with InternalErrorWithDescription instead of classifying the user as unreachable (which produced a misleading UnreachableBackends error for e.g. brig 5xx failures). - InternalCommit.processInternalCommit: a collapsed addMLSClients child aborts the commit instead of silently dropping client-store writes. Adds CheckClientsSpec covering both the crash abort (local user) and the unaffected federation-hush path (remote user), mirroring the production interpreter layering. InternalError now derives Show for test diagnostics.
- CheckClients.checkClients requires Member (Error InternalError) and aborts with InternalErrorWithDescription when sequenceConcurrently collapses a brig client-data child to Nothing, instead of treating it as 'user unreachable' (misleading UnreachableBackends). - processInternalCommit aborts the commit when a concurrent addMLSClients child returns Nothing instead of silently dropping client-store writes. - Registers CheckClientsSpec in the wire-subsystems test suite; InternalError derives Show for test diagnostics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://wearezeta.atlassian.net/browse/WPB-28483
Checklist
changelog.d