fix(event-buffer): release consolidated events in upstream's order - #51
Merged
Conversation
A flush walks `Object.keys()` of the consolidated map, so the order `consolidateEvents` writes its keys is the order a `process()` handler iterates and the order the individual events reach `.on()` listeners. This buffer wrote contacts and groups before the message kinds, where upstream writes them after, so a consumer whose handlers assume upstream's sequence saw a different interleaving. No event was lost — the conservation target was always clean — but the ordering is part of what "upstream-compatible" means for this buffer. Reordering the writes is the whole fix; the work done per flush is unchanged. The differential entry that documented the divergence is removed, and the merge-precedence predicate it composed with is tightened from multiset pairing to positional pairing, since a reordering is a finding again rather than something an entry excuses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe event buffer now emits message events before contact and group events. A compatibility test compares local ordering with the upstream implementation. Differential merge checks now pair events by position and reject reordered sequences. ChangesEvent ordering compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
2 participants
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.
Summary
makeEventBufferreleased the consolidated kinds in a different order than upstream. A flush walksObject.keys()of the mapconsolidateEventsbuilds, so that map's insertion order is two observable things at once: the key order aprocess()handler iterates, and the order the individual events are re-dispatched to.on()listeners.Measured against
baileys/lib/Utils/event-buffer.json a buffer holding all eleven kinds:No event was lost —
buffer:conservationwas always clean — but a consumer whose handlers assume upstream's sequence (messages released before the contacts that reference them) saw a different interleaving. The fix reorders the writes inconsolidateEventsto match upstream.Root cause
consolidateEventsgrouped its writes by helper rather than by upstream's sequence: everyassignArray(...)call ran first, then the four branches that build a non-array payload (messages.upsert,messages.delete,messages.reaction,message-receipt.update). Upstream interleaves the two. The writes are now interleaved the same way, with a comment saying the order is the contract so a later tidy-up does not regroup them.Registry
This closes
event-buffer-release-order, which documented the divergence.isPermutation, its only predicate, goes with it.event-buffer-merge-precedencecomposed with it: that entry paired released observations by kind and identity rather than by position, precisely because a field difference and a reordering co-occurred. With the release sequences aligned that slack is unjustified — an entry that still tolerated a reordering would be the thing excusing one — somergePrecedenceFieldsOnlynow pairs positionally and fails outright if a release moved. The harness test that asserted the old composition asserts the opposite; two near-miss pins around it are unchanged.Registry: 21 open / 25 total → 20 open / 24 total.
Validation
The new test reads the expectation from upstream at run time rather than hardcoding the list — a frozen list would go stale the moment upstream reorders and would then assert compatibility with a version nobody runs. It also asserts the seed actually produced all eleven kinds, so the order it pins is not the order of whatever survived consolidation.
Negative test. Reverting only the reordering in
consolidateEventsand re-running:Performance
The change moves statements; it adds no allocation, no branch and no call. Measured to confirm that, rather than asserted.
Harness: one emitter reused across iterations (
flush()swaps in a fresh buffer), so each iteration is exactlybuffer()→ 11emit()→ consolidate → release, with no emitter construction diluting the signal. 20k warm-up iterations, then the measured window. The two implementations alternate within one process so they share JIT warmth and any drift in machine state.Allocation pressure — 500k flushes,
--trace-gc, three independent process pairs:Scavenge count is identical in every pair — young-generation allocation volume over 500k flushes is unchanged to the GC event, which is the strongest form this measurement takes. RSS varies by ±4% across runs of the same implementation, so it carries no signal here.
Throughput and CPU — 300k flushes per sample, 8 alternating samples each:
The 0.8% median gap is inside the run-to-run spread of either implementation (13%), and the ranges overlap almost completely — the fastest "after" sample beats six of the eight "before" samples. No regression.
Not in this PR
Running the buffer differential in deep mode surfaces one further divergence, present on
mainbefore this change (verified by stashing): acontacts.updatebuffered before amessaging-history.setfor the same id resolves to a different name than upstream's (""here, the later upsert's value upstream). Fixed-seed mode — what pull requests run — is green on both sides. Worth its own investigation rather than widening this one.Generated by Claude Code
Summary by cubic
Fixes the event buffer to release consolidated events in the same order as upstream
baileys, so message events come before related contacts/groups. This aligns handler iteration and listener dispatch order with upstream and removes the temporary ordering allowlist.Bug Fixes
consolidateEventsto match upstream key insertion order used byObject.keys()during flush.Tests & Harness
baileys/lib/Utils/event-buffer.jsand asserts exact match.event-buffer-release-orderallowlist andisPermutation; tightenedmergePrecedenceFieldsOnlyto be position-sensitive and updated harness tests accordingly.Written for commit de2874f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests