fix: streamline join event creation and enhance error messaging for e… #201
Conversation
…vent ID mismatches
|
Warning Rate limit exceeded@ggazzo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughRefactors join event creation to use StateService.buildEvent with inline auth events, makes StateService.addAuthEvents private, improves an error message, and reworks event hashing in EventWrapper to avoid mutating raw events while deduplicating event references for deterministic content hashing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant SendJoinService
participant StateService
participant Persistence as Persistence/Signer
Client->>SendJoinService: sendJoin(request)
SendJoinService->>StateService: buildEvent<'m.room.member'>(raw + {auth_events: []}, roomVersion)
rect rgba(200,230,255,0.3)
note right of StateService: Internally gathers auth events (addAuthEvents now private)
end
StateService-->>SendJoinService: Built event
SendJoinService->>StateService: Fetch state / validate eventId
SendJoinService->>Persistence: Persist + Sign + Emit
Persistence-->>Client: Response
sequenceDiagram
autonumber
participant Wrapper as EventWrapper
participant Hasher as ContentHash
Wrapper->>Wrapper: getEventWithoutHashes()<br/>• strip hashes/signatures<br/>• dedupe auth_events/prev_events
Wrapper->>Hasher: getContentHash(merge(rawEvent, sanitized))
Hasher-->>Wrapper: sha256
Wrapper-->>Caller: event with computed hashes.sha256<br/>(signatures/unsigned preserved)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
==========================================
+ Coverage 81.14% 81.18% +0.04%
==========================================
Files 63 63
Lines 4666 4676 +10
==========================================
+ Hits 3786 3796 +10
Misses 880 880 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/federation-sdk/src/services/state.service.ts (1)
286-289: Error message improvement is good; add more context (state_key) for faster triage.Including the state_key clarifies which mapping caused the mismatch during backfills/debugging.
- throw new Error( - `Event id mismatch while building room state ${pdu.eventId} !== ${eventId}`, - ); + throw new Error( + `Event id mismatch while building room state (state_key=${stateKey}): built=${pdu.eventId} stored=${eventId}`, + );packages/room/src/manager/event-wrapper.ts (1)
113-121: Guard against undefined arrays in auth_events/prev_events.If either field is absent (or null), spreading will throw. Default to [] before dedup.
- return { - ...event, - auth_events: Array.from(new Set([...this.rawEvent.auth_events])), - prev_events: Array.from(new Set([...this.rawEvent.prev_events])), - }; + return { + ...event, + auth_events: Array.from(new Set(this.rawEvent.auth_events ?? [])), + prev_events: Array.from(new Set(this.rawEvent.prev_events ?? [])), + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/federation-sdk/src/services/send-join.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(2 hunks)packages/room/src/manager/event-wrapper.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/federation-sdk/src/services/send-join.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
event(102-111)
packages/federation-sdk/src/services/state.service.ts (2)
packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/room/src/manager/event-wrapper.ts (1)
event(102-111)
packages/room/src/manager/event-wrapper.ts (1)
packages/room/src/types/_common.ts (1)
PduForType(11-11)
🔇 Additional comments (2)
packages/federation-sdk/src/services/send-join.service.ts (1)
32-35: Drop redundant second signing — use buildEvent result directlybuildEvent(...) already returns a signed event; remove the later stateService.signEvent(joinEvent) to avoid double-signing and signature drift.
- Action: Remove stateService.signEvent(joinEvent) and use joinEvent where signedJoinEvent is currently used (packages/federation-sdk/src/services/send-join.service.ts — buildEvent around lines 32–35; extra sign around lines 68–81).
- Optional: rename joinEvent -> signedJoinEvent for clarity.
packages/federation-sdk/src/services/state.service.ts (1)
425-436: No external callsites found — addAuthEvents may remain private.Only occurrence is this.addAuthEvents(instance) in packages/federation-sdk/src/services/state.service.ts:417; method defined at line 425.
| return { | ||
| ...this.rawEvent, | ||
| ...this.getEventWithoutHashes(), | ||
| hashes: { | ||
| sha256: toUnpaddedBase64(this.getContentHash()), | ||
| }, | ||
| signatures: this.signatures, | ||
| unsigned: this.rawEvent.unsigned ?? {}, | ||
| } as PduForType<Type>; |
There was a problem hiding this comment.
🧩 Analysis chain
Dedup before hashing may change remote PDU IDs/hashes — gate dedup to locally-built events.
get event() now returns a sanitized event (deduped auth_events/prev_events) and uses that for hashing. If this wrapper is used for inbound federated PDUs, deduping prior to hashing deviates from “hash what you received” and can yield event_id/content hash mismatches against remote servers.
- Proposal: Only apply dedup/sanitization when constructing local events (e.g., when rawEvent.hashes is absent). For received PDUs (hashes present), compute hashes from rawEvent as-is.
Add tests covering:
- Remote PDU with intentionally duplicated auth_events/prev_events retains original event_id/content hash.
- Locally built PDU dedupes deterministically and remains stable under repeated access.
Would you like me to draft these tests to close the patch coverage gap reported by Codecov?
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== repo root files (top-level) ==="
ls -la || true
echo -e "\n=== locate event-wrapper.ts in repo ==="
rg -n --hidden --no-ignore-vcs -S 'event-wrapper\.ts' || true
echo -e "\n=== search for relevant symbols across repo ==="
rg -n --hidden --no-ignore-vcs -S 'getEventWithoutHashes|getContentHash|toUnpaddedBase64|rawEvent|auth_events|prev_events|dedup|dedupe|sanitize|get event\(|get event\s*:' || true
echo -e "\n=== list packages/room directory ==="
ls -la packages || true
ls -la packages/room || true
ls -la packages/room/src || true
ls -la packages/room/src/manager || true
EVENT_FILE='packages/room/src/manager/event-wrapper.ts'
if [ -f "$EVENT_FILE" ]; then
echo -e "\n=== show $EVENT_FILE (first 400 lines) ==="
sed -n '1,400p' "$EVENT_FILE"
else
echo -e "\nfile not found at $EVENT_FILE; showing any files under packages/room/src/manager"
rg -n --hidden --no-ignore-vcs -S '' packages/room/src/manager || true
fi
echo -e "\n=== show implementations/occurrences of getContentHash and getEventWithoutHashes ==="
rg -n --hidden --no-ignore-vcs -S 'getContentHash\s*\(|function getContentHash|getEventWithoutHashes' || true
echo -e "\n=== show occurrences of toUnpaddedBase64 ==="
rg -n --hidden --no-ignore-vcs -S 'toUnpaddedBase64' || true
echo -e "\n=== search for any code branching on rawEvent.hashes or presence of hashes ==="
rg -n --hidden --no-ignore-vcs -S 'rawEvent\.hashes|\\.hashes\\s*\\?|rawEvent\\[\"hashes\"\\]|if\\s*\\(.*hashes' || true
echo -e "\n=== search for sanitization/dedup helper names ==="
rg -n --hidden --no-ignore-vcs -S 'sanitizeEvent|sanitize|dedup|dedupe|deduplicate|uniquePrev|uniqueAuth' || true
echo -e "\n=== end ==="Length of output: 57066
Critical: don't dedupe auth_events/prev_events before hashing for received PDUs
get event() and the instance getContentHash() currently merge a deduped event (auth_events/prev_events) into rawEvent before computing/returning the sha256, which will recompute/override hashes for inbound PDUs and can cause event_id/content-hash mismatches with remote servers. See packages/room/src/manager/event-wrapper.ts — get event() (≈lines 102–110), getEventWithoutHashes() (≈113–121), instance getContentHash() (≈325–333).
- Fix: only apply dedup/sanitization for locally-built events (e.g., when rawEvent.hashes is absent). For received PDUs (rawEvent.hashes present) compute and return hashes from rawEvent as-is (no dedup).
- Tests to add:
- inbound PDU with duplicated auth_events/prev_events must retain the original event_id/content-hash provided by the sender;
- locally-built PDU must dedupe deterministically and remain stable across repeated accesses.
I can draft those tests if helpful.
🤖 Prompt for AI Agents
In packages/room/src/manager/event-wrapper.ts around lines 103–110, the current
event() and getContentHash() logic merges a deduped version of
auth_events/prev_events into rawEvent before computing/returning sha256, which
alters hashes for inbound PDUs; change the logic so that if rawEvent.hashes
exists (i.e., this is a received PDU) you must not modify or dedupe rawEvent at
all and must compute/return hashes directly from rawEvent as-is (or simply
return the provided hashes), otherwise (locally-built PDUs where rawEvent.hashes
is absent) apply the deterministic dedupe/sanitization before computing hashes;
update getEventWithoutHashes()/getContentHash() accordingly and add tests: 1)
inbound PDU with duplicated auth_events/prev_events preserves sender’s
event_id/content-hash, and 2) locally-built PDU dedupes deterministically and
remains stable across accesses.
| getContentHash() { | ||
| return PersistentEventBase.getContentHash(this.rawEvent); | ||
| return PersistentEventBase.getContentHash({ | ||
| ...this.rawEvent, | ||
| ...this.getEventWithoutHashes(), // basically make sure the prev and auth events are not duplicated | ||
| }); | ||
| } |
There was a problem hiding this comment.
Content-hash computation: prefer raw for remote PDUs; sanitized for local.
Today you merge sanitized into raw before hashing, which changes the hash when duplicates exist. Make the path explicit: use rawEvent if hashes exist (assume remote), otherwise use sanitized for deterministic local construction.
- return PersistentEventBase.getContentHash({
- ...this.rawEvent,
- ...this.getEventWithoutHashes(), // basically make sure the prev and auth events are not duplicated
- });
+ // If the event already has hashes, assume it is a remote PDU and hash exactly what we received.
+ // Otherwise (local construction), hash the sanitized/deduped version for determinism.
+ const toHash =
+ this.rawEvent.hashes
+ ? this.rawEvent
+ : { ...this.rawEvent, ...this.getEventWithoutHashes() };
+ return PersistentEventBase.getContentHash(toHash);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getContentHash() { | |
| return PersistentEventBase.getContentHash(this.rawEvent); | |
| return PersistentEventBase.getContentHash({ | |
| ...this.rawEvent, | |
| ...this.getEventWithoutHashes(), // basically make sure the prev and auth events are not duplicated | |
| }); | |
| } | |
| getContentHash() { | |
| // If the event already has hashes, assume it is a remote PDU and hash exactly what we received. | |
| // Otherwise (local construction), hash the sanitized/deduped version for determinism. | |
| const toHash = | |
| this.rawEvent.hashes | |
| ? this.rawEvent | |
| : { ...this.rawEvent, ...this.getEventWithoutHashes() }; | |
| return PersistentEventBase.getContentHash(toHash); | |
| } |
3 participants
…vent ID mismatches
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Refactor