perf(binary): move the WABinary codec to the Rust bridge - #1698
perf(binary): move the WABinary codec to the Rust bridge#1698jlucaso1 wants to merge 12 commits into
Conversation
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
jlucaso1
added
help wanted
| jid: string | ||
| type: 'pkmsg' | 'msg' | ||
| ciphertext: Uint8Array | ||
| ciphertext: Buffer |
There was a problem hiding this comment.
This is needed because libsignal and WAGroup rely heavy on NodeJs Buffer (this will change in the libsignal rewrite too), but the new implementation of binary-wasm rely on pure Uint8Array.
There was a problem hiding this comment.
How about we shift libsignal and WAGroup logic away from Buffers before merging this? This will allow us to make it more Web & other runtime compatible without causing unnecessary changes that will be reverted in the code. If you can update the current old and rusty WASignalGroup & the current libsignal, it is OK enough.
There was a problem hiding this comment.
This can be more risky and there is the fact that the libsignal repository is not versioned correctly, which can lead to problems in older versions of baileys.
There was a problem hiding this comment.
This can be more risky and there is the fact that the libsignal repository is not versioned correctly, which can lead to problems in older versions of baileys.
That's correct, I failed to notice that. We'll make the changes then revert them.
|
Can you benchmark this? Mock test sendNode and pass through a thousand or so nodes and also make sure to make some different nodes chock-full of data and fields and test the difference. |
| "music-metadata": "^11.7.0", | ||
| "pino": "^9.6", | ||
| "protobufjs": "^7.2.4", | ||
| "whatsapp-rust-bridge": "^0.1.3", |
There was a problem hiding this comment.
Can we please not have any external dependencies? I'd like if this & its repo was within the WhiskeySockets GitHub&NPM orgs, to prevent any malicious intent and to allow for quick and easy maintenance
| jid: string | ||
| type: 'pkmsg' | 'msg' | ||
| ciphertext: Uint8Array | ||
| ciphertext: Buffer |
There was a problem hiding this comment.
How about we shift libsignal and WAGroup logic away from Buffers before merging this? This will allow us to make it more Web & other runtime compatible without causing unnecessary changes that will be reverted in the code. If you can update the current old and rusty WASignalGroup & the current libsignal, it is OK enough.
|
bench.zip #using deprecated parameters for the initialization function; pass a single object instead # need to solve this warning later
clk: ~4.21 GHz
cpu: 12th Gen Intel(R) Core(TM) i5-12450H
runtime: node 22.18.0 (x64-linux)
benchmark avg (min … max) p75 / p99 (min … top 1%)
------------------------------------------- -------------------------------
• Encoding (JS Object -> Binary)
------------------------------------------- -------------------------------
Rust WASM (warmup-ignore) 45.61 µs/iter 50.37 µs ▅█
(34.73 µs … 119.46 µs) 70.90 µs ▄███▅██▅▃▇▄▃▃▂▃▂▃▃▁▁▁
gc( 2.15 ms … 4.51 ms) 3.59 kb ( 3.11 kb… 26.16 kb)
0.36 ipc ( 46.18% cache) 740.92 branch misses
129.58k cycles 46.62k instructions 4.70k c-refs 2.53k c-misses
Rust WASM (small) 5.25 µs/iter 5.34 µs █▂▂█ ▆▂
(4.97 µs … 5.92 µs) 5.90 µs ████▃▅▅██▇▁▅▁▁▃▃▃▃▁▁▃
gc( 2.24 ms … 5.43 ms) 3.11 kb ( 3.09 kb… 3.52 kb)
3.27 ipc ( 69.45% cache) 17.71 branch misses
18.94k cycles 61.92k instructions 67.10 c-refs 20.50 c-misses
Rust WASM (medium) 8.77 µs/iter 9.44 µs █ ▅
(7.14 µs … 12.38 µs) 11.45 µs ▇█▄▄▄▁█▇▇▇▄▇▄▄▄▁▁▄▁▁▄
gc( 2.41 ms … 6.44 ms) 218.68 b (214.03 b…364.67 b)
3.30 ipc ( 64.43% cache) 23.56 branch misses
27.61k cycles 91.22k instructions 97.38 c-refs 34.64 c-misses
Rust WASM (large) 184.65 µs/iter 219.50 µs ▇█▃
(128.17 µs … 358.25 µs) 332.92 µs ▃███▆▆▂▃▄▄▃▅▃▂▂▄▂▂▁▁▁
gc( 3.04 ms … 8.93 ms) 36.75 kb ( 29.88 kb… 77.92 kb)
1.75 ipc ( 35.92% cache) 1.68k branch misses
397.69k cycles 695.75k instructions 6.54k c-refs 4.19k c-misses
Old js (warmup-ignore) 66.52 µs/iter 71.58 µs ▃██▃▂▃
(46.67 µs … 178.23 µs) 126.36 µs ▃██████▆▄▅▃▂▁▂▂▁▂▁▁▁▁
gc( 3.00 ms … 6.82 ms) 9.19 kb ( 8.58 kb… 56.84 kb)
0.52 ipc ( 43.39% cache) 941.18 branch misses
175.45k cycles 91.09k instructions 6.32k c-refs 3.58k c-misses
Old js (small) 14.14 µs/iter 14.43 µs ▃ ▃ █ ▃
(13.26 µs … 15.20 µs) 15.07 µs ▆▁█▆▁▆▁▆█▆█▆▁▆█▆▁▁▁▁▆
gc( 2.37 ms … 3.62 ms) 2.88 kb ( 2.84 kb… 3.47 kb)
3.58 ipc ( 57.80% cache) 32.29 branch misses
48.83k cycles 174.80k instructions 391.15 c-refs 165.06 c-misses
Old js (medium) 20.62 µs/iter 20.71 µs █ █
(20.34 µs … 20.77 µs) 20.75 µs █▁▁▁▁▁▁█▁▁▁███▁██▁█▁█
gc( 2.49 ms … 4.26 ms) 2.59 kb ( 2.53 kb… 2.74 kb)
3.68 ipc ( 57.36% cache) 48.53 branch misses
71.93k cycles 264.83k instructions 622.16 c-refs 265.31 c-misses
Old js (large) 371.65 µs/iter 401.43 µs ▅█▃▄ ▇
(264.36 µs … 750.28 µs) 665.32 µs ▄▇██████▆▆▂▂▁▂▂▁▂▁▁▁▂
gc( 2.11 ms … 9.01 ms) 358.00 kb ( 2.30 kb…944.30 kb)
2.62 ipc ( 53.75% cache) 3.10k branch misses
1.18M cycles 3.08M instructions 16.05k c-refs 7.42k c-misses |
|
Solved the warning message. The only missing thing is the transfer of the npm package to WhiskeySockets |
|
So rust bridge is only about 2x faster than the original JS version, right? Libsignal, for example, eats way more CPU time anyway. |
The Rust implementation is sill unoptimized, so there is huge potential for this. We already see a 3x improvement in small encodings, which is better for connection stability and speed as well.
We are planning to move libsignal under this rust bridge, and WASM allows for all runtime support, so it is a good place to be. We are already learning how to deal with Rust here for libsignal, so the node support is alright. |
Some users of baileys are using worker threads, because of the high load of encoding/deconding. Vini:
Agree, libsignal is the heaviest, but the binary things are in second place. |
In my use case (hundred of groups) libsignal consumes a lot of CPU in Base64/Buffer encode/decode. Binary encoding its the heaviest part to me. |
|
i rewrited this using pure js and it's now x10 faster... |
share with us |
share with us |
|
I've made some optimizations (reducing allocations and removed some unecessary operations in the rust code). Improved execution time and memory usage in operations. New benchmark results (bench.zip): ➜ Baileys git:(feat-binary-node-rust-wasm) ✗ yarn tsx --expose-gc bench/bench.ts
clk: ~4.18 GHz
cpu: 12th Gen Intel(R) Core(TM) i5-12450H
runtime: node 22.18.0 (x64-linux)
benchmark avg (min … max) p75 / p99 (min … top 1%)
------------------------------------------- -------------------------------
• Encoding (JS Object -> Binary)
------------------------------------------- -------------------------------
Rust WASM (small) 4.92 µs/iter 4.95 µs ▄█▅▄
(4.68 µs … 5.17 µs) 5.14 µs ▃▁▁▁▃▆▄▆▄████▆▃▁▄▃▁▁▃
gc( 2.17 ms … 3.48 ms) 2.47 kb ( 2.47 kb… 2.60 kb)
3.36 ipc ( 74.49% cache) 16.01 branch misses
18.56k cycles 62.41k instructions 48.50 c-refs 12.37 c-misses
Rust WASM (medium) 7.28 µs/iter 7.31 µs ▄ █ ▄▄ ▄█
(7.20 µs … 7.36 µs) 7.35 µs ▅▅▅█▁█▅█▅█▅██▅██▁█▅▅▅
gc( 2.21 ms … 3.19 ms) 3.52 kb ( 3.52 kb… 3.52 kb)
3.47 ipc ( 74.73% cache) 22.98 branch misses
27.23k cycles 94.37k instructions 66.81 c-refs 16.89 c-misses
Rust WASM (large) 59.83 µs/iter 59.93 µs █ █ █
(59.51 µs … 60.24 µs) 60.07 µs ██▁▁▁▁▁▁█▁██▁▁▁█▁▁▁██
gc( 2.28 ms … 3.21 ms) 1.01 kb ( 1.01 kb… 1.01 kb)
3.61 ipc ( 75.05% cache) 254.49 branch misses
222.48k cycles 802.85k instructions 542.33 c-refs 135.31 c-misses
Old js (small) 88.79 µs/iter 90.01 µs █▇
(76.21 µs … 158.24 µs) 140.38 µs ▅███▅▃▁▂▂▃▄▃▂▂▂▁▁▁▁▁▁
gc( 2.10 ms … 4.12 ms) 27.64 kb ( 22.95 kb…119.06 kb)
1.14 ipc ( 56.55% cache) 1.53k branch misses
271.81k cycles 309.33k instructions 7.89k c-refs 3.43k c-misses
Old js (medium) 19.43 µs/iter 19.46 µs █
(19.26 µs … 19.60 µs) 19.59 µs █▁▁▅▅▁▁▁▁▅▅▁█▁▁▁▁▅▁▁▅
gc( 2.39 ms … 4.97 ms) 1.96 kb ( 1.88 kb… 2.21 kb)
3.68 ipc ( 59.53% cache) 56.31 branch misses
71.58k cycles 263.50k instructions 607.44 c-refs 245.82 c-misses
Old js (large) 352.57 µs/iter 389.67 µs ▂▃ ▆█▃ ▇▄▄▆▂▄ ▂▅
(258.33 µs … 520.43 µs) 447.75 µs ▄▃██▅██████████▅██▆▄▄
gc( 2.32 ms … 8.13 ms) 391.87 kb ( 54.12 kb… 1.22 mb)
2.67 ipc ( 55.72% cache) 3.18k branch misses
1.19M cycles 3.18M instructions 15.88k c-refs 7.03k c-misses |
b3dc623 to
c443704
Compare
vague claims. |
|
Now everything is working (after the changes of LID stuffs, somethings has broken, but all has solved now). The only missing on this PR is to improve the DX and reduce the breaking changes. Basically I've found that we can use rollup (bundler) in https://github.com/WhiskeySockets/whatsapp-rust-bridge that will drop the necessity of calling initWasm (async function in init of the scoket creation). The unique drawback is because the bundle of the library will become 30% larger (currently is about 180kb, btw the perfomance will not be changed). |
|
Finished, now no breaking changes in the API, e2e tests are passing too. Final bundle package is around 240kb only Ready for intensive testing |
github-project-automation
Bot
moved this from In progress
to Done
in Baileys Review Cycle
|
Tested and working |
|
Can we compare this implementation with this PR #2513 ? |
|
Bechmark with #2513 PR |
3bf8903 to
96971c7
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
jlucaso1
changed the title
Both directions crossed the boundary per field. Decoding exposed a handle whose every getter went back into WASM, and encoding let Rust pull the node apart through Reflect; that was 56% of the encode profile, against 12% doing the work. The handle path lost to the TypeScript decoder by 9.9x on a 64-participant stanza. decodeNodeFlat and encodeNodeFlat share one buffer format instead: u32 stringBytes, offsetCount, layoutCount, blobBytes string data | u32 offsets | u32 layout | blob data Sections are 4-aligned so the reader takes views in place. Four things made it fast, each from a profile rather than a guess: - Tag and attribute strings are interned; converting them again on each occurrence was 13% of decode CPU. - The builder is reused across calls; four allocations per decode showed up as allocator time. - FxHash keyed on content, so interning does not allocate a String per distinct tag. - Known tags travel as an index into the token table, which tokenTable() exposes once, rather than as bytes. This is what closed the gap on small stanzas, where fixed cost dominates. Inflation is deliberately left to the caller: doing it here runs zlib synchronously and stalls the loop long enough on a compressed group stanza to miss deadlines.
Both directions now go through the flat buffer format, and the hand
written encoder, decoder and token tables come out: 1,600 lines less to
keep in step with the wire format.
Against the TypeScript they replace, on a group stanza with 1, 8 and 64
participants, interleaved in one process so drift hits both sides:
encode 2.11x 2.80x 3.04x faster
decode 1.05x 1.12x 1.20x faster
Encoder output is byte-identical, which the recorded parity vectors
check against the JS implementation. Decoded content comes back as a
Buffer rather than a bare Uint8Array, so callers doing
`(node.content as Buffer).toString('utf-8')` keep working: an earlier
handle-based attempt broke pairing on exactly that.
Inflation stays here rather than in the bridge, because node runs zlib
on the thread pool and the bridge would run it inline.
The binary bench compared `encodeNode` and `decodeNode` against rc.9, but Baileys stopped calling either: it goes through the flat codec. It also timed `decodeNode` on its own, which returns a handle that materializes on access, so the fastest line in the file measured a decode that had not happened. Bring in the shipped encoder and decoder by path and run them over an ack, a one to one message, a device fanout at 8 and 64, and an app state patch, compressed and not. The handle path keeps one line, against the flat path, to show what it costs to read a whole tree through it. binary-tree.ts goes: it existed to compare the handle and flat paths on a group stanza, which this now covers.
Returning the flat buffer as a `Uint8Array` cost around 500ns a call, near
enough fixed: a JS typed array to allocate, a wasm-bindgen heap slot to
take it through, and a fifth buffer to concatenate the four sections into
first. On a 31 byte ack that was most of the decode, and it is why the
smallest and most frequent stanzas lost to the TypeScript decoder while the
large ones won.
The decoder now leaves its sections where it wrote them and publishes their
offsets to a fixed address, which the caller reads out of linear memory. No
concatenation, no JS object per call, and the alignment the u32 views need
comes from the element type rather than from padding. What escapes the call
is byte content, so the decoder copies that section once and leaves the
leaves as views into the copy.
Three more, found by profiling what was left:
- FxHash walked a byte at a time and was 23% of the decode. Eight.
- `ValueRef::as_str` builds a `String` for every jid attribute, one per
participant on a fanout. Format straight into the pool instead.
- Clearing the intern table cost the whole table, so a group stanza grew it
and every small stanza after that paid to wipe it. Age entries by round.
On the JS side the reader state moves out of the closures the decode
captured, the encoder keeps its four collections across calls, the frame is
copied once rather than twice, and an uncompressed frame no longer takes a
second async hop to find out it needs no inflating.
Measured against baileys@7.0.0-rc.9, pinned to a P core:
decode ack 0.55us vs 0.69 | direct 1.96 vs 2.34 | fanout8 10.3 vs 12.9
fanout64 62.7 vs 79.5 | app state 8.5 vs 12.0
encode ack 1.08us vs 1.15 | direct 1.73 vs 4.88 | fanout8 6.6 vs 22.0
fanout64 40.9 vs 131.8 | app state 15.9 vs 129.3
The flat codec had no test of its own, only a bench, so it gets one: wire
parity against a recorded stanza, each content kind, the token boundary,
interning across rounds, and the two lifetime rules the borrow depends on.
A jid with no user renders as a bare `s.whatsapp.net` in the core, and the TypeScript decoder this replaced kept the `@`. Baileys routes on that: `handleEncryptNotification` matches `from === S_WHATSAPP_NET`, so the server's own pre-key count was falling through to the identity-change branch and replenishment never ran. Two round-trip tests had recorded the broken output as their expectation, and re-encoding a decoded frame now reproduces it byte for byte again, matching rc.9 exactly. The encoder also stringified attributes that were null or undefined, putting the literal text `undefined` on the wire where the previous encoder omitted the attribute. Optional attributes reach it unset from real call sites, a USync user queried by phone and a media retry with no participant among them. The count is backfilled rather than taken from a filtered array, which would be an allocation per node and cost 15% of a device fanout. Two more, neither reachable from Baileys but both public: - `encodeNodeFlat` derived its section spans with unchecked arithmetic, so a count whose byte length wraps on wasm32 passed the bounds check and then built a slice reaching far past the buffer. It also cast the byte spans to `*const u32`, which needs an alignment the caller's allocation does not promise. Both go: spans are checked, and words are read with `from_le_bytes`, which wasm does unaligned at no cost. - `toJSON` walked the parsed node while the getters returned whatever a setter had put in their place, so a mutated handle serialized a stale tree. It now serializes what was written, and keeps the single-pass walk for a node nobody has written to.
`toJSON` walking the parsed node instead of the getters, which is how the previous implementation read it, drops more than an assignment over `node.attrs`. The object a getter hands back is live, and so is a child handle, so `node.attrs.x = '...'` and a write to a child were both lost while `encodeNode` and the getters themselves saw them. Taking the caches whenever they are populated covers all three, and it replaces the written flags from the previous commit, which only ever caught assignment. A node nobody has touched still takes the single pass over the parsed tree, which is every reader. The encoder also lost two guards the TypeScript one had. An empty tag was interned and shipped as a malformed stanza rather than throwing, and a child left out of a conditionally built list arrives as null, which the previous encoder dropped and this one crashed on while reading `tag` off it. Both now behave as before, byte for byte against rc.9.
Picks up three that landed upstream: a byte-pair table for the packed values, `smoothutf8` in place of `std` for validating what that table just wrote, and dropping the per-node box around `NodeRef::content`. The third one is breaking. `content` went from `Option<Box<NodeContentRef>>` to `Option<NodeContentRef>`, so the three reads become `as_ref` and the three sites in `FlatReader` stop boxing what they build. No logic moves. Measured against the previous pin, alternating both builds three times with a stanza that carries neither packed values nor content as a control for drift: the ack is unchanged, a device fanout is around 5% faster. The same commits measure well ahead of that natively, which is the third time this decoder has gained less on wasm than on the host.
The first case in the file absorbed the JIT warm-up for the whole run, worth around 150ns, and it landed on whichever stanza happened to be first. That read as the smallest stanza being the one case the bridge lost: 660ns against 620 for the TypeScript decoder. Moving the ack out of first position put it at 497 against 695, and moved the penalty onto whatever took its place. With both decoders warmed over every fixture first, the ack decodes in 473ns against 572, and every other case keeps the standing it had.
The table that dedups strings within a decode aged its entries out by a round counter instead of removing them, so every distinct message id, jid and timestamp a socket ever decoded stayed in it. Two million decodes took linear memory from 1.3 MB to 274 MB, growing in doublings as hashbrown rehashed, and WASM never returns memory to the host. It is now flat at 1.3 MB across the same run. Clearing the map each decode would also bound it, and that is what the code did before; it was changed because clearing costs the whole table, so one group stanza grew it and every small stanza after paid to wipe the buckets left behind. A fixed 512-slot direct-mapped table has neither problem: it cannot grow, it never needs clearing because the round stamp still scopes hits to one decode, and a lookup is one indexed load instead of a hashbrown probe. Dedup was already best effort, which is what makes eviction on collision sound: the caller writes the string twice and the pool gets longer. The index bound in the hit check also covers the round counter wrapping after four billion decodes onto a stale entry, which would have indexed out of range. Found by an audit agent reading the allocation behaviour, not by a benchmark: the benchmarks decode the same three frames in a loop, so the table stays at four entries and the growth is invisible to them.
…hat hides growth Two hazards in the borrowed-memory contract, found by an audit agent reading the lifetime rules rather than by a test. A stanza with no byte content leaves the blob section empty, and the decoder stood in the whole-memory view for it. A node whose content was an empty byte string therefore came back as a zero-length Buffer whose `buffer` was all of linear memory at offset zero: nothing readable through the view itself, but a live handle to the heap that Signal keys live in, handed to application code. It now gets its own empty backing store. The cached views are rebuilt when a growth detaches them, which is detected as a length of zero. That is only sound for a plain, non-resizable ArrayBuffer: a SharedArrayBuffer never detaches, and a resizable one is tracked by some view types and not others, so the check would report "still valid" and the decoder would serve truncated strings with no error. The runtime now refuses either at init. The check reads the two properties directly instead of using `instanceof`, which does not hold across realms: under jest's VM context the host ArrayBuffer is not the context's, and the first version rejected every buffer in the test suite.
…a dead hash `promiseTimeout` captured `new Error().stack` and so did `delayCancellable`, which it calls, so every outbound frame paid for two. Capturing the frames costs about 1.2us; reading `.stack` makes V8 format them and costs about 11us. Worse, the formatting was not confined to the timeout path: settling ran `cancel`, which built a Boom and read its stack, so a successful send paid the full price. `promiseTimeout` only ever needed the timer dropped, so `delayCancellable` grows a `clear` for that, and both errors now expose the stack through a getter so the frames are formatted only if something reads them. That takes a `promiseTimeout` call from 22.0us to 3.0us. Every send goes through one and every query through two. `generateParticipantHashV2` was also being run per send and discarded: `extraAttrs` was already spread into the `<enc>` nodes by the two awaited `createParticipantNodes` calls above it, and nothing reads it afterwards. Fixing the order would not have helped either, since the participant hash belongs on the message stanza rather than on `<enc>`. Removed rather than moved, because putting it on the wire is a protocol change and not a performance one.
96971c7 to
f3bfaaf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
packages/baileys/src/WABinary/encode.ts (1)
104-147: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRelease the retained payload buffers after serialization.
flattenclears the shared state at the start of a call, not at the end. Between calls,strings,blobs, andseenstill reference every payload from the previous encode. A large media blob therefore stays reachable until the nextencodeBinaryNodecall, which can be a long idle period. The output buffer is already an independent copy, so the arrays can be cleared before returning.♻️ Proposed refactor to drop references before returning
for (const bytes of blobs) { bytes.copy(out, at) at += bytes.length } + // The output is an independent copy, so drop the payload references here + // instead of holding them until the next call. + strings.length = 0 + blobs.length = 0 + seen.clear() + return out }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/baileys/src/WABinary/encode.ts` around lines 104 - 147, Update flatten so the shared strings, blobs, and seen state is cleared after the output buffer has been fully serialized and before returning it; preserve the existing output construction and return the independent buffer unchanged.packages/whatsapp-rust-bridge/src/binary.rs (3)
380-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe doc comment on
token_tabledescribes the flat buffer, not the token table.Lines 381-390 document a single buffer that holds the decoded tree and a header of
stringBytes,offsetCount,layoutCount,blobBytes. That header is the input layoutencode_node_flatreads at lines 816-832. The decode output is published as four(offset, length)pairs bypublish. Only the last paragraph, from line 390, applies totoken_table.Move the layout paragraph to
encode_node_flator to theFlatResultdoc, and leave the token-table paragraph here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/whatsapp-rust-bridge/src/binary.rs` around lines 380 - 393, Correct the doc comment attached to token_table so it only documents the flattened token table and its boot-time lookup behavior. Move the decoded-tree buffer layout description, including the header fields and sections, to encode_node_flat or FlatResult, preserving accurate documentation for both symbols.
128-153: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe intern cache key set includes attribute values, so the bound is reached by user data.
Line 165 interns
value_string(v). Attribute values include JIDs, message IDs, and timestamps. Those are high-cardinality. The cache therefore fills with 512 single-use entries and clears repeatedly, which drops the hot tag and attribute-name entries with them. The comment states the working set is the token table and not user data, which does not hold for values.Consider interning only attribute names and tags, and creating values with
JsValue::from_strdirectly. Alternatively, gate interning onindex_of_token(value).is_some(), so only known tokens enter the cache.♻️ Proposed change to keep the cache bounded to token-like strings
fn intern(value: &str) -> JsValue { + // Only strings drawn from the token table repeat across stanzas. A jid or + // message id is seen once, and admitting it evicts the entries that pay. + if index_of_token(value).is_none() { + return JsValue::from_str(value); + } + INTERNED.with(|cache| {Also applies to: 165-165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/whatsapp-rust-bridge/src/binary.rs` around lines 128 - 153, Update the value-conversion path around intern and the call at value_string(v) so user-provided attribute values are created directly with JsValue::from_str instead of entering INTERNED. Keep interning for tags and attribute names only, or gate intern with index_of_token(value).is_some(), ensuring INTERNED remains limited to known token strings.
743-755: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
FlatReader::readrecurses without a depth bound.Content kind
3callsself.read()for each child at line 773. The child count and the nesting come from the caller's layout buffer. A layout that nests deeply drives Rust stack recursion until the WASM stack is exhausted, which traps the instance rather than returning aJsValueerror.The Baileys encoder at
packages/baileys/src/WABinary/encode.tsbuilds the layout from aBinaryNodetree, so the depth is bounded in practice by that tree. The reader is still reachable from__encodeNodeFlat, which is an exported entry point that accepts arbitrary bytes. Consider carrying a depth counter onFlatReaderand returningbad_index()past a fixed limit.Also applies to: 765-786
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/whatsapp-rust-bridge/src/binary.rs` around lines 743 - 755, The recursive FlatReader::read path lacks protection against deeply nested input from __encodeNodeFlat. Add a depth counter or equivalent recursion limit to FlatReader, increment it while reading nested content kind 3 children, and return bad_index() once a fixed maximum depth is exceeded; preserve normal parsing for inputs within the limit.packages/whatsapp-rust-bridge/test/flat.test.ts (3)
412-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet an explicit timeout on the 60,000-decode test.
Line 425 runs 60,000 decodes, and line 432 runs a further 5,000. Jest applies a 5-second default timeout per test. The assertion is a memory-growth regression guard, so the iteration count cannot be reduced without weakening it. Pass an explicit timeout to
itso a slow or loaded CI runner does not report a false failure.♻️ Proposed explicit timeout
- it("does not grow across decodes that share no strings", () => { + // 60k decodes is the point at which the old leak was measurable, so the + // count sets the timeout rather than the other way round. + it("does not grow across decodes that share no strings", () => {Then extend the
itcall at line 428:}, 30_000);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/whatsapp-rust-bridge/test/flat.test.ts` around lines 412 - 428, Extend the `it` call for “does not grow across decodes that share no strings” with an explicit 30-second timeout, preserving the 60,000-decode regression-guard iteration count and assertion.
30-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
builddoes not mirror the Baileys consumer for the no-blob case.Line 29 states this helper mirrors the assembly in Baileys. Lines 45-47 fall back to
byteswhenblobBytesis zero.bytesis a Buffer over the whole WASM heap.packages/baileys/src/WABinary/decode.tsline 87 uses a zero-lengthNO_BLOBSfor the same case, precisely so a leaf does not get a view over all of linear memory.The two differ only when a stanza has no blob section, and then no kind-1 leaf exists, so no current assertion changes. Align the helper anyway, so it keeps its stated role as the oracle for the consumer.
♻️ Proposed alignment with the Baileys consumer
const blobs = blobBytes ? Buffer.from(bytes.subarray(blobsAt, blobsAt + blobBytes)) - : bytes; + : Buffer.alloc(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/whatsapp-rust-bridge/test/flat.test.ts` around lines 30 - 49, Update build so the no-blob case creates and uses a zero-length blob buffer, matching Baileys’ NO_BLOBS behavior, instead of falling back to the full bytes buffer. Preserve the existing subarray behavior when blobBytes is present and keep the change scoped to the blobs initialization in build.
353-370: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend the malformed-input coverage to the blob span.
These two cases cover the header counts that
checked_bytesguards. The blob span inFlatReader::readis not covered.packages/whatsapp-rust-bridge/src/binary.rsline 762 addsoffsetandlenwithout a check, and both words come from this same caller-supplied buffer. A test that writes a kind-1 leaf whoseoffset + lenwraps on wasm32 would pin the fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/whatsapp-rust-bridge/test/flat.test.ts` around lines 353 - 370, Extend the malformed-input test in “rejects a header whose section length overflows” to construct a kind-1 leaf with blob offset and length values whose sum overflows on wasm32, then assert encodeNodeFlat throws. Use the existing buffer layout conventions and ensure both words are sourced from the caller-supplied buffer, covering the unchecked blob span calculation in FlatReader::read.packages/baileys/src/WABinary/decode.ts (1)
74-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the synchronous window explicit so a later
awaitcannot corrupt the shared state.
pool,words,blobs, andcursorare module-level. The code is correct today: the onlyawaitis on line 75, beforedecodeNodeFlat, and lines 76-89 run in one synchronous block. Concurrent calls therefore cannot interleave.The invariant is not enforced. If a later change adds an
awaitbetweendecodeNodeFlatandread(), a second decode overwrites the state and the first call returns a tree assembled from two frames. The comment at lines 17-20 records the requirement. Consider moving the assembly into a named synchronous function that takes the flat node, so the boundary is visible in the code and not only in the comment.♻️ Proposed split of the async prefix from the synchronous assembly
-export const decodeBinaryNode = async (buff: Buffer): Promise<BinaryNode> => { - const body = 2 & buff.readUInt8() ? await inflatePromise(buff.subarray(1)) : buff.subarray(1) - const flat = decodeNodeFlat(body) +// Runs to completion without yielding, which is what lets the reader state +// above be shared across calls. +const assemble = (body: Buffer): BinaryNode => { + const flat = decodeNodeFlat(body) const { bytes, stringsAt, offsetsAt, offsetCount, blobsAt, blobBytes } = flat words = flat.words @@ blobs = blobBytes ? Buffer.from(bytes.subarray(blobsAt, blobsAt + blobBytes)) : NO_BLOBS cursor = flat.layoutAt return read() } + +export const decodeBinaryNode = async (buff: Buffer): Promise<BinaryNode> => + assemble(2 & buff.readUInt8() ? await inflatePromise(buff.subarray(1)) : buff.subarray(1))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/baileys/src/WABinary/decode.ts` around lines 74 - 89, Make the synchronous state-assembly portion of decodeBinaryNode explicit by extracting the logic after the existing await into a named synchronous helper that accepts the result of decodeNodeFlat. Keep pool, words, blobs, cursor initialization and read() within that helper without introducing any await, and have decodeBinaryNode only perform decompression and delegate the flat node to it.packages/whatsapp-rust-bridge/benches/binary.ts (1)
136-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe warm-up is asymmetric:
walkis warmed only for the JS tree shape.Line 142 walks the tree that
decodeOldreturns. Line 140 decodes throughdecodeBinaryNodebut does not walk the result.walkis a polymorphic reader, so the shapes it sees decide which inline caches V8 builds. Thedecode+walkgroup at lines 173-181 then measures the WASM side withwalkspecialized on the JS shape only.The comment at lines 132-135 states the intent is to remove first-case bias. Add the missing warm call to keep the two paths comparable.
♻️ Proposed fix to warm `walk` on both tree shapes
do_not_optimize(await decodeBinaryNode(warm)); do_not_optimize(await decodeOld(warm)); + do_not_optimize(walk(await decodeBinaryNode(warm))); do_not_optimize(walk((await decodeOld(warm)) as BinaryNode));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/whatsapp-rust-bridge/benches/binary.ts` around lines 136 - 144, Update the warm-up loop around encodeBinaryNode and decodeBinaryNode to also pass the decoded binary result to walk, alongside the existing decodeOld-and-walk call. Keep both tree shapes warmed before the benchmark measurements so walk receives each decoder’s output shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/baileys/src/WABinary/encode.ts`:
- Around line 90-95: Update the binary-content branch in the encoder to accept
only Buffer/Uint8Array or string-compatible content before calling Buffer.from,
and reject unsupported values explicitly. Include the current node’s tag in the
thrown failure while preserving the existing child-node and string handling
paths.
In `@packages/whatsapp-rust-bridge/benches/binary.ts`:
- Around line 206-210: Update the “decode+walk fanout 8 (wasm handle)” benchmark
to free every child handle produced while traversing via walk, not just the root
handle. Ensure cleanup occurs for all handles returned through the content
getter before or alongside freeing the root, while preserving the benchmark’s
measured decode-and-walk behavior.
In `@packages/whatsapp-rust-bridge/src/binary.rs`:
- Around line 757-764: Update the bytes branch in the content parsing match
around NodeContentRef::Bytes to compute the blob range end with checked_add
instead of offset + len, converting overflow into bad_index and preserving the
existing get-based validation before borrowing the slice.
---
Nitpick comments:
In `@packages/baileys/src/WABinary/decode.ts`:
- Around line 74-89: Make the synchronous state-assembly portion of
decodeBinaryNode explicit by extracting the logic after the existing await into
a named synchronous helper that accepts the result of decodeNodeFlat. Keep pool,
words, blobs, cursor initialization and read() within that helper without
introducing any await, and have decodeBinaryNode only perform decompression and
delegate the flat node to it.
In `@packages/baileys/src/WABinary/encode.ts`:
- Around line 104-147: Update flatten so the shared strings, blobs, and seen
state is cleared after the output buffer has been fully serialized and before
returning it; preserve the existing output construction and return the
independent buffer unchanged.
In `@packages/whatsapp-rust-bridge/benches/binary.ts`:
- Around line 136-144: Update the warm-up loop around encodeBinaryNode and
decodeBinaryNode to also pass the decoded binary result to walk, alongside the
existing decodeOld-and-walk call. Keep both tree shapes warmed before the
benchmark measurements so walk receives each decoder’s output shape.
In `@packages/whatsapp-rust-bridge/src/binary.rs`:
- Around line 380-393: Correct the doc comment attached to token_table so it
only documents the flattened token table and its boot-time lookup behavior. Move
the decoded-tree buffer layout description, including the header fields and
sections, to encode_node_flat or FlatResult, preserving accurate documentation
for both symbols.
- Around line 128-153: Update the value-conversion path around intern and the
call at value_string(v) so user-provided attribute values are created directly
with JsValue::from_str instead of entering INTERNED. Keep interning for tags and
attribute names only, or gate intern with index_of_token(value).is_some(),
ensuring INTERNED remains limited to known token strings.
- Around line 743-755: The recursive FlatReader::read path lacks protection
against deeply nested input from __encodeNodeFlat. Add a depth counter or
equivalent recursion limit to FlatReader, increment it while reading nested
content kind 3 children, and return bad_index() once a fixed maximum depth is
exceeded; preserve normal parsing for inputs within the limit.
In `@packages/whatsapp-rust-bridge/test/flat.test.ts`:
- Around line 412-428: Extend the `it` call for “does not grow across decodes
that share no strings” with an explicit 30-second timeout, preserving the
60,000-decode regression-guard iteration count and assertion.
- Around line 30-49: Update build so the no-blob case creates and uses a
zero-length blob buffer, matching Baileys’ NO_BLOBS behavior, instead of falling
back to the full bytes buffer. Preserve the existing subarray behavior when
blobBytes is present and keep the change scoped to the blobs initialization in
build.
- Around line 353-370: Extend the malformed-input test in “rejects a header
whose section length overflows” to construct a kind-1 leaf with blob offset and
length values whose sum overflows on wasm32, then assert encodeNodeFlat throws.
Use the existing buffer layout conventions and ensure both words are sourced
from the caller-supplied buffer, covering the unchecked blob span calculation in
FlatReader::read.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 357a9a47-b423-4c78-9b7f-4f1d67801173
⛔ Files ignored due to path filters (1)
packages/whatsapp-rust-bridge/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
packages/baileys/src/Socket/messages-send.tspackages/baileys/src/Utils/generics.tspackages/baileys/src/WABinary/constants.tspackages/baileys/src/WABinary/decode.tspackages/baileys/src/WABinary/encode.tspackages/baileys/src/WABinary/types.tspackages/baileys/src/__tests__/Utils/generics.test.tspackages/baileys/src/__tests__/binary/wabinary-codec.test.tspackages/whatsapp-rust-bridge/benches/binary.tspackages/whatsapp-rust-bridge/src/binary.rspackages/whatsapp-rust-bridge/test/binary.test.tspackages/whatsapp-rust-bridge/test/flat.test.tspackages/whatsapp-rust-bridge/test/handshake-parity.test.tspackages/whatsapp-rust-bridge/test/parity.test.tspackages/whatsapp-rust-bridge/test/server-response-parity.test.tspackages/whatsapp-rust-bridge/ts/flat.tspackages/whatsapp-rust-bridge/ts/index.cjs.tspackages/whatsapp-rust-bridge/ts/index.tspackages/whatsapp-rust-bridge/ts/wasm-runtime.ts
💤 Files with no reviewable changes (3)
- packages/baileys/src/WABinary/types.ts
- packages/baileys/src/Socket/messages-send.ts
- packages/baileys/src/WABinary/constants.ts
Two wire and packaging regressions from the migration. An `INTEROP_JID` carries an integrator, and the core's `Display` leaves it out: a jid with user `abc`, device 42 and integrator 7 came back as `abc:42@interop` where the decoder this replaced wrote `7-abc:42@interop`. Distinct interop identities collapse onto one string, which downstream session and message routing keys on. Both the flat path and the handle now go through one helper, so the two spellings cannot drift; the server-only `@` case moved into it as well. `ts/index.ts` re-exports `./flat.js`, so the generated `dist/index.d.ts` references `dist/flat.d.ts`, which the `files` list did not carry. The failure is quiet: esbuild inlines the module so the runtime works, and TypeScript drops the unresolvable re-export and reports `decodeNodeFlat` and `encodeNodeFlat` as simply not exported, suggesting the raw `__decodeNodeFlat` instead. Confirmed against a real `npm pack`. `check-package` now walks the relative re-exports of every packed declaration and fails when one does not resolve inside the tarball, so the class of bug cannot come back for the next file.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/baileys/src/__tests__/Utils/generics.test.ts">
<violation number="1" location="packages/baileys/src/__tests__/Utils/generics.test.ts:158">
P3: The second test overwrites the process-global `Error.prepareStackTrace` and assumes nothing else in the worker formats a stack for the duration of the call. It is restored in `finally` and jest runs tests in a file sequentially, so this is currently safe, but mutating a global V8 hook from a unit test is fragile: any concurrent or newly added code that reads `.stack` would route through this counting function and flake the `formatted === 0` assertion. Consider documenting that isolation boundary (or scoping the check so it can't be inflated by unrelated stack reads) so the perf intent survives future test additions.</violation>
</file>
<file name="packages/whatsapp-rust-bridge/scripts/check-package.mjs">
<violation number="1" location="packages/whatsapp-rust-bridge/scripts/check-package.mjs:175">
P2: Valid relative declaration imports without a `.js` suffix (or using `.mjs`/`.cjs`) make `test:package` fail as “not packed”; resolving the declaration with the usual extension and index candidates would keep this packaging check from rejecting valid TypeScript output.</violation>
<violation number="2" location="packages/whatsapp-rust-bridge/scripts/check-package.mjs:175">
P2: The new declaration-resolution guard compares paths produced by OS-native `join`/`dirname` against `npm pack --json` file paths, which are always forward-slash separated. On Windows these don't match (`dist\flat.d.ts` vs `dist/flat.d.ts`), so the check would spuriously throw `... is not packed` for every valid re-export and break `pnpm test:package` for Windows developers. Normalizing the computed target to forward slashes (and appending `.d.ts` for the bundler-style extensionless specifiers the tsconfig's `moduleResolution: bundler` permits) keeps the guard correct across platforms.</violation>
</file>
<file name="packages/whatsapp-rust-bridge/src/binary.rs">
<violation number="1" location="packages/whatsapp-rust-bridge/src/binary.rs:749">
P2: Out-of-range token indices are accepted and can wrap into a different valid double-token entry during `encodeNodeFlat`. Reject token values above the four exported double dictionaries before the `u8` casts so malformed input cannot change the wire node silently.</violation>
<violation number="2" location="packages/whatsapp-rust-bridge/src/binary.rs:775">
P2: `encodeNodeFlat` can be made to allocate memory proportional to an arbitrary count field instead of rejecting a malformed buffer. In `FlatReader::read`, `Vec::with_capacity(attr_count)` (and the matching `Vec::with_capacity(count)` for children) sizes an allocation directly from a u32 read out of the caller's layout, before verifying the layout actually holds that many entries. A header advertising a huge `attr_count`/`count` with only a short layout triggers an enormous pre-allocation that aborts the WASM module rather than returning the 'layout ran out' error the other malformed frames produce. Since this entry point's contract (per its own tests) is to reject unparseable input cleanly, sizing the collection from the actual available layout bytes (or using a checked/saturating capacity) would keep the failure path cheap and predictable.</violation>
</file>
<file name="packages/whatsapp-rust-bridge/test/handshake-parity.test.ts">
<violation number="1" location="packages/whatsapp-rust-bridge/test/handshake-parity.test.ts:401">
P2: The hardcoded expected bytes in this round-trip test are only self-checked against WASM's own encode/decode, not against the recorded rc.9 legacy vectors, because this exact node appears in no encode-parity test and has no vector in legacy-wire-vectors.json. As a result a wire-format divergence from rc.9 for this frame shape would pass silently as long as WASM remains internally consistent, despite the PR's parity claim. Consider adding an encode-parity (expectByteIdentical) assertion for this node so the snapshot is anchored to the rc.9 reference like the pair-device round-trip test is.</violation>
</file>
<file name="packages/whatsapp-rust-bridge/benches/binary.ts">
<violation number="1" location="packages/whatsapp-rust-bridge/benches/binary.ts:117">
P3: In the `walk` helper, the `?? content` fallback in `do_not_optimize((content as Uint8Array).length ?? content)` is dead: at that branch `content` can only be a `string` or `Uint8Array`, both of which have a numeric `.length`, so the fallback is never reached (and `do_not_optimize` is never given the content itself). For string leaves the `Uint8Array` cast is also misleading and `.length` reports UTF-16 code-unit count rather than bytes. Consider simplifying to `do_not_optimize(content.length)` (or, to actually exercise each leaf the way the socket will, passing the content through `do_not_optimize`) so the branch reflects what is being measured.</violation>
</file>
<file name="packages/baileys/src/Socket/messages-send.ts">
<violation number="1" location="packages/baileys/src/Socket/messages-send.ts:936">
P2: This change removes the `phash` attribute from the wire stanza, not just a local computation. `extraAttrs` is spread into the `<enc>` node (messages-send.ts:829) and every participant node (messages-send.ts:928-929), so group/participant messages go out without the participant-hash attribute they previously carried, and the receive path still references `attrs.phash` (messages-recv.ts:1974-1975). The PR description lists this as an unverified 'separable change' bundled into a perf PR. Since this is a protocol-level behavior change, please validate it against WhatsApp Web/source semantics (or confirm the server path that consumes `phash` is truly unused) before merging, rather than removing it on an assumption; if the protocol requires the hash for group membership sync, this can silently break group participant handling.</violation>
</file>
<file name="packages/baileys/src/Utils/generics.ts">
<violation number="1" location="packages/baileys/src/Utils/generics.ts:150">
P3: For the exported `delayCancellable`, `cancel` now captures its stack lazily at the moment `cancel()` is invoked rather than when the delay was created. For the `promiseTimeout` path this is fine (it hoists `origin` at entry and uses `clear`), but a consumer calling `.cancel()` later from a different module/async context will get a stack from the cancel-call site — not the original caller — which defeats the stated purpose of 'reporting where the caller was' and regresses the previous behavior. Consider capturing `const origin = callerFrames()` at the top of `delayCancellable` (next to `clear`) and referencing it inside `cancel`, so both `cancel` and the timeout paths report the delay creator's frames.</violation>
</file>
<file name="packages/baileys/src/WABinary/decode.ts">
<violation number="1" location="packages/baileys/src/WABinary/decode.ts:8">
P3: After the inflate logic was inlined into `decodeBinaryNode`, the exported `decompressingIfRequired` helper still carries the identical decompression code but has no remaining callers inside the package. It's a duplicated code path that can drift from the inline version if the inflate behavior ever changes (e.g. throwing vs. handling the 0x00 prefix). Consider removing the standalone helper and keeping only the inline branch in `decodeBinaryNode` (or delegating the inline branch to it) so there is a single source of truth for frame decompression.</violation>
<violation number="2" location="packages/baileys/src/WABinary/decode.ts:78">
P3: The decoder's per-call state (`pool`, `words`, `blobs`, `cursor`) lives at module scope and is shared across every concurrent `decodeBinaryNode` call, even though the function is `async` and can suspend on `inflatePromise`. It currently works only because all four bindings are reassigned after the await and `read()` runs synchronously with no intervening await. This is a fragile invariant for a library that decodes frames from multiple sockets at once: if anyone later adds an `await` inside `read()` or between the reassignments and the read, stale/WASM memory from a previous call would be read, silently corrupting decoded nodes. Consider encapsulating this per-decode state in a small context object allocated inside `decodeBinaryNode` (and passed to `read`) so correctness doesn't depend on the absence of an await and on a fresh blob buffer being allocated each call.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| for (const declaration of declarations) { | ||
| const source = readFileSync(join(root, declaration), 'utf8') | ||
| for (const [, specifier] of source.matchAll(/(?:from|import)\s*["'](\.[^"']+)["']/g)) { | ||
| const target = join(dirname(declaration), specifier).replace(/\.js$/, '.d.ts') |
There was a problem hiding this comment.
P2: Valid relative declaration imports without a .js suffix (or using .mjs/.cjs) make test:package fail as “not packed”; resolving the declaration with the usual extension and index candidates would keep this packaging check from rejecting valid TypeScript output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/whatsapp-rust-bridge/scripts/check-package.mjs, line 175:
<comment>Valid relative declaration imports without a `.js` suffix (or using `.mjs`/`.cjs`) make `test:package` fail as “not packed”; resolving the declaration with the usual extension and index candidates would keep this packaging check from rejecting valid TypeScript output.</comment>
<file context>
@@ -157,6 +157,29 @@ function hasInlineWasm(path) {
+ for (const declaration of declarations) {
+ const source = readFileSync(join(root, declaration), 'utf8')
+ for (const [, specifier] of source.matchAll(/(?:from|import)\s*["'](\.[^"']+)["']/g)) {
+ const target = join(dirname(declaration), specifier).replace(/\.js$/, '.d.ts')
+ if (!packedFiles.has(target)) {
+ throw new Error(`${declaration} re-exports ${specifier}, but ${target} is not packed`)
</file context>
| const target = join(dirname(declaration), specifier).replace(/\.js$/, '.d.ts') | |
| const target = [ | |
| join(dirname(declaration), specifier).replace(/\.js$/, '.d.ts'), | |
| join(dirname(declaration), specifier).replace(/\.mjs$/, '.d.mts'), | |
| join(dirname(declaration), specifier).replace(/\.cjs$/, '.d.cts'), | |
| join(dirname(declaration), `${specifier}.d.ts`), | |
| join(dirname(declaration), specifier, 'index.d.ts') | |
| ].find(target => packedFiles.has(target)) ?? join(dirname(declaration), specifier) |
| if index >= TOKEN_BASE { | ||
| let token = index - TOKEN_BASE; | ||
| let text = if token < 256 { | ||
| get_single_token(token as u8) | ||
| } else { | ||
| let flat = token - 256; | ||
| get_double_token((flat / 256) as u8, (flat % 256) as u8) | ||
| }; | ||
|
|
There was a problem hiding this comment.
P2: Out-of-range token indices are accepted and can wrap into a different valid double-token entry during encodeNodeFlat. Reject token values above the four exported double dictionaries before the u8 casts so malformed input cannot change the wire node silently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/whatsapp-rust-bridge/src/binary.rs, line 749:
<comment>Out-of-range token indices are accepted and can wrap into a different valid double-token entry during `encodeNodeFlat`. Reject token values above the four exported double dictionaries before the `u8` casts so malformed input cannot change the wire node silently.</comment>
<file context>
@@ -274,3 +402,476 @@ pub fn decode_node(data: Vec<u8>) -> Result<InternalBinaryNode, JsValue> {
+ }
+
+ fn str_at(&self, index: u32) -> Result<NodeStr<'a>, JsValue> {
+ if index >= TOKEN_BASE {
+ let token = index - TOKEN_BASE;
+ let text = if token < 256 {
</file context>
| if index >= TOKEN_BASE { | |
| let token = index - TOKEN_BASE; | |
| let text = if token < 256 { | |
| get_single_token(token as u8) | |
| } else { | |
| let flat = token - 256; | |
| get_double_token((flat / 256) as u8, (flat % 256) as u8) | |
| }; | |
| if index >= TOKEN_BASE { | |
| let token = index - TOKEN_BASE; | |
| let text = if token < 256 { | |
| get_single_token(token as u8) | |
| } else if token < 256 + u32::from(DOUBLE_DICTS) * 256 { | |
| let flat = token - 256; | |
| get_double_token((flat / 256) as u8, (flat % 256) as u8) | |
| } else { | |
| return Err(JsValue::from_str("flat encode: unknown token index")); | |
| }; |
| expect(hex(encoded2)).toBe(hex(encoded1)); | ||
| expect(hex(encoded2)).toBe( | ||
| `00f80a1916cb045a110308fc0e746573742d726f756e6474726970f802f802adfc04000000fef8029cfc04abcd1234` | ||
| `00f80a1916cb045a11fa000308fc0e746573742d726f756e6474726970f802f802adfc04000000fef8029cfc04abcd1234` |
There was a problem hiding this comment.
P2: The hardcoded expected bytes in this round-trip test are only self-checked against WASM's own encode/decode, not against the recorded rc.9 legacy vectors, because this exact node appears in no encode-parity test and has no vector in legacy-wire-vectors.json. As a result a wire-format divergence from rc.9 for this frame shape would pass silently as long as WASM remains internally consistent, despite the PR's parity claim. Consider adding an encode-parity (expectByteIdentical) assertion for this node so the snapshot is anchored to the rc.9 reference like the pair-device round-trip test is.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/whatsapp-rust-bridge/test/handshake-parity.test.ts, line 401:
<comment>The hardcoded expected bytes in this round-trip test are only self-checked against WASM's own encode/decode, not against the recorded rc.9 legacy vectors, because this exact node appears in no encode-parity test and has no vector in legacy-wire-vectors.json. As a result a wire-format divergence from rc.9 for this frame shape would pass silently as long as WASM remains internally consistent, despite the PR's parity claim. Consider adding an encode-parity (expectByteIdentical) assertion for this node so the snapshot is anchored to the rc.9 reference like the pair-device round-trip test is.</comment>
<file context>
@@ -392,8 +396,9 @@ describe("Round-trip Parity (Encode→Decode→Encode)", () => {
+ expect(hex(encoded2)).toBe(hex(encoded1));
expect(hex(encoded2)).toBe(
- `00f80a1916cb045a110308fc0e746573742d726f756e6474726970f802f802adfc04000000fef8029cfc04abcd1234`
+ `00f80a1916cb045a11fa000308fc0e746573742d726f756e6474726970f802f802adfc04000000fef8029cfc04abcd1234`
);
});
</file context>
| participants.push(...otherNodes) | ||
|
|
||
| if (meRecipients.length > 0 || otherRecipients.length > 0) { | ||
| extraAttrs['phash'] = generateParticipantHashV2([...meRecipients, ...otherRecipients]) |
There was a problem hiding this comment.
P2: This change removes the phash attribute from the wire stanza, not just a local computation. extraAttrs is spread into the <enc> node (messages-send.ts:829) and every participant node (messages-send.ts:928-929), so group/participant messages go out without the participant-hash attribute they previously carried, and the receive path still references attrs.phash (messages-recv.ts:1974-1975). The PR description lists this as an unverified 'separable change' bundled into a perf PR. Since this is a protocol-level behavior change, please validate it against WhatsApp Web/source semantics (or confirm the server path that consumes phash is truly unused) before merging, rather than removing it on an assumption; if the protocol requires the hash for group membership sync, this can silently break group participant handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/baileys/src/Socket/messages-send.ts, line 936:
<comment>This change removes the `phash` attribute from the wire stanza, not just a local computation. `extraAttrs` is spread into the `<enc>` node (messages-send.ts:829) and every participant node (messages-send.ts:928-929), so group/participant messages go out without the participant-hash attribute they previously carried, and the receive path still references `attrs.phash` (messages-recv.ts:1974-1975). The PR description lists this as an unverified 'separable change' bundled into a perf PR. Since this is a protocol-level behavior change, please validate it against WhatsApp Web/source semantics (or confirm the server path that consumes `phash` is truly unused) before merging, rather than removing it on an assumption; if the protocol requires the hash for group membership sync, this can silently break group participant handling.</comment>
<file context>
@@ -932,10 +931,6 @@ export const makeMessagesSocket = (config: SocketConfig) => {
-
shouldIncludeDeviceIdentity = shouldIncludeDeviceIdentity || s1 || s2
}
</file context>
| let tag_index = self.next()?; | ||
| let tag = self.str_at(tag_index)?; | ||
| let attr_count = self.next()? as usize; | ||
| let mut attrs = Vec::with_capacity(attr_count); |
There was a problem hiding this comment.
P2: encodeNodeFlat can be made to allocate memory proportional to an arbitrary count field instead of rejecting a malformed buffer. In FlatReader::read, Vec::with_capacity(attr_count) (and the matching Vec::with_capacity(count) for children) sizes an allocation directly from a u32 read out of the caller's layout, before verifying the layout actually holds that many entries. A header advertising a huge attr_count/count with only a short layout triggers an enormous pre-allocation that aborts the WASM module rather than returning the 'layout ran out' error the other malformed frames produce. Since this entry point's contract (per its own tests) is to reject unparseable input cleanly, sizing the collection from the actual available layout bytes (or using a checked/saturating capacity) would keep the failure path cheap and predictable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/whatsapp-rust-bridge/src/binary.rs, line 775:
<comment>`encodeNodeFlat` can be made to allocate memory proportional to an arbitrary count field instead of rejecting a malformed buffer. In `FlatReader::read`, `Vec::with_capacity(attr_count)` (and the matching `Vec::with_capacity(count)` for children) sizes an allocation directly from a u32 read out of the caller's layout, before verifying the layout actually holds that many entries. A header advertising a huge `attr_count`/`count` with only a short layout triggers an enormous pre-allocation that aborts the WASM module rather than returning the 'layout ran out' error the other malformed frames produce. Since this entry point's contract (per its own tests) is to reject unparseable input cleanly, sizing the collection from the actual available layout bytes (or using a checked/saturating capacity) would keep the failure path cheap and predictable.</comment>
<file context>
@@ -274,3 +402,476 @@ pub fn decode_node(data: Vec<u8>) -> Result<InternalBinaryNode, JsValue> {
+ let tag_index = self.next()?;
+ let tag = self.str_at(tag_index)?;
+ let attr_count = self.next()? as usize;
+ let mut attrs = Vec::with_capacity(attr_count);
+ for _ in 0..attr_count {
+ let key_index = self.next()?;
</file context>
| // the formatting this change defers, not the capture. | ||
| const previous = Error.prepareStackTrace | ||
| let formatted = 0 | ||
| Error.prepareStackTrace = (err, frames) => { |
There was a problem hiding this comment.
P3: The second test overwrites the process-global Error.prepareStackTrace and assumes nothing else in the worker formats a stack for the duration of the call. It is restored in finally and jest runs tests in a file sequentially, so this is currently safe, but mutating a global V8 hook from a unit test is fragile: any concurrent or newly added code that reads .stack would route through this counting function and flake the formatted === 0 assertion. Consider documenting that isolation boundary (or scoping the check so it can't be inflated by unrelated stack reads) so the perf intent survives future test additions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/baileys/src/__tests__/Utils/generics.test.ts, line 158:
<comment>The second test overwrites the process-global `Error.prepareStackTrace` and assumes nothing else in the worker formats a stack for the duration of the call. It is restored in `finally` and jest runs tests in a file sequentially, so this is currently safe, but mutating a global V8 hook from a unit test is fragile: any concurrent or newly added code that reads `.stack` would route through this counting function and flake the `formatted === 0` assertion. Consider documenting that isolation boundary (or scoping the check so it can't be inflated by unrelated stack reads) so the perf intent survives future test additions.</comment>
<file context>
@@ -129,3 +131,41 @@ describe('runDetached', () => {
+ // the formatting this change defers, not the capture.
+ const previous = Error.prepareStackTrace
+ let formatted = 0
+ Error.prepareStackTrace = (err, frames) => {
+ formatted++
+ return previous ? previous(err, frames) : frames.join('\n')
</file context>
| do_not_optimize(content); | ||
| for (const child of content) seen += walk(child as BinaryNode); | ||
| } else if (content) { | ||
| do_not_optimize((content as Uint8Array).length ?? content); |
There was a problem hiding this comment.
P3: In the walk helper, the ?? content fallback in do_not_optimize((content as Uint8Array).length ?? content) is dead: at that branch content can only be a string or Uint8Array, both of which have a numeric .length, so the fallback is never reached (and do_not_optimize is never given the content itself). For string leaves the Uint8Array cast is also misleading and .length reports UTF-16 code-unit count rather than bytes. Consider simplifying to do_not_optimize(content.length) (or, to actually exercise each leaf the way the socket will, passing the content through do_not_optimize) so the branch reflects what is being measured.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/whatsapp-rust-bridge/benches/binary.ts, line 117:
<comment>In the `walk` helper, the `?? content` fallback in `do_not_optimize((content as Uint8Array).length ?? content)` is dead: at that branch `content` can only be a `string` or `Uint8Array`, both of which have a numeric `.length`, so the fallback is never reached (and `do_not_optimize` is never given the content itself). For string leaves the `Uint8Array` cast is also misleading and `.length` reports UTF-16 code-unit count rather than bytes. Consider simplifying to `do_not_optimize(content.length)` (or, to actually exercise each leaf the way the socket will, passing the content through `do_not_optimize`) so the branch reflects what is being measured.</comment>
<file context>
@@ -1,127 +1,215 @@
- do_not_optimize(content);
+ for (const child of content) seen += walk(child as BinaryNode);
+ } else if (content) {
+ do_not_optimize((content as Uint8Array).length ?? content);
+ seen++;
}
</file context>
| do_not_optimize((content as Uint8Array).length ?? content); | |
| do_not_optimize(content.length); |
| const clear = () => clearTimeout(timeout) | ||
| const cancel = () => { | ||
| clearTimeout(timeout) | ||
| const origin = callerFrames() |
There was a problem hiding this comment.
P3: For the exported delayCancellable, cancel now captures its stack lazily at the moment cancel() is invoked rather than when the delay was created. For the promiseTimeout path this is fine (it hoists origin at entry and uses clear), but a consumer calling .cancel() later from a different module/async context will get a stack from the cancel-call site — not the original caller — which defeats the stated purpose of 'reporting where the caller was' and regresses the previous behavior. Consider capturing const origin = callerFrames() at the top of delayCancellable (next to clear) and referencing it inside cancel, so both cancel and the timeout paths report the delay creator's frames.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/baileys/src/Utils/generics.ts, line 150:
<comment>For the exported `delayCancellable`, `cancel` now captures its stack lazily at the moment `cancel()` is invoked rather than when the delay was created. For the `promiseTimeout` path this is fine (it hoists `origin` at entry and uses `clear`), but a consumer calling `.cancel()` later from a different module/async context will get a stack from the cancel-call site — not the original caller — which defeats the stated purpose of 'reporting where the caller was' and regresses the previous behavior. Consider capturing `const origin = callerFrames()` at the top of `delayCancellable` (next to `clear`) and referencing it inside `cancel`, so both `cancel` and the timeout paths report the delay creator's frames.</comment>
<file context>
@@ -125,27 +125,43 @@ export const debouncedTimeout = (intervalMs = 1000, task?: () => void) => {
+ const clear = () => clearTimeout(timeout)
const cancel = () => {
- clearTimeout(timeout)
+ const origin = callerFrames()
+ clear()
reject(
</file context>
| const body = 2 & buff.readUInt8() ? await inflatePromise(buff.subarray(1)) : buff.subarray(1) | ||
| const flat = decodeNodeFlat(body) | ||
| const { bytes, stringsAt, offsetsAt, offsetCount, blobsAt, blobBytes } = flat | ||
| words = flat.words |
There was a problem hiding this comment.
P3: The decoder's per-call state (pool, words, blobs, cursor) lives at module scope and is shared across every concurrent decodeBinaryNode call, even though the function is async and can suspend on inflatePromise. It currently works only because all four bindings are reassigned after the await and read() runs synchronously with no intervening await. This is a fragile invariant for a library that decodes frames from multiple sockets at once: if anyone later adds an await inside read() or between the reassignments and the read, stale/WASM memory from a previous call would be read, silently corrupting decoded nodes. Consider encapsulating this per-decode state in a small context object allocated inside decodeBinaryNode (and passed to read) so correctness doesn't depend on the absence of an await and on a fresh blob buffer being allocated each call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/baileys/src/WABinary/decode.ts, line 78:
<comment>The decoder's per-call state (`pool`, `words`, `blobs`, `cursor`) lives at module scope and is shared across every concurrent `decodeBinaryNode` call, even though the function is `async` and can suspend on `inflatePromise`. It currently works only because all four bindings are reassigned after the await and `read()` runs synchronously with no intervening await. This is a fragile invariant for a library that decodes frames from multiple sockets at once: if anyone later adds an `await` inside `read()` or between the reassignments and the read, stale/WASM memory from a previous call would be read, silently corrupting decoded nodes. Consider encapsulating this per-decode state in a small context object allocated inside `decodeBinaryNode` (and passed to `read`) so correctness doesn't depend on the absence of an await and on a fresh blob buffer being allocated each call.</comment>
<file context>
@@ -1,308 +1,90 @@
+ const body = 2 & buff.readUInt8() ? await inflatePromise(buff.subarray(1)) : buff.subarray(1)
+ const flat = decodeNodeFlat(body)
+ const { bytes, stringsAt, offsetsAt, offsetCount, blobsAt, blobBytes } = flat
+ words = flat.words
- return {
</file context>
| } | ||
|
|
||
| return buffer | ||
| export const decompressingIfRequired = async (buffer: Buffer) => |
There was a problem hiding this comment.
P3: After the inflate logic was inlined into decodeBinaryNode, the exported decompressingIfRequired helper still carries the identical decompression code but has no remaining callers inside the package. It's a duplicated code path that can drift from the inline version if the inflate behavior ever changes (e.g. throwing vs. handling the 0x00 prefix). Consider removing the standalone helper and keeping only the inline branch in decodeBinaryNode (or delegating the inline branch to it) so there is a single source of truth for frame decompression.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/baileys/src/WABinary/decode.ts, line 8:
<comment>After the inflate logic was inlined into `decodeBinaryNode`, the exported `decompressingIfRequired` helper still carries the identical decompression code but has no remaining callers inside the package. It's a duplicated code path that can drift from the inline version if the inflate behavior ever changes (e.g. throwing vs. handling the 0x00 prefix). Consider removing the standalone helper and keeping only the inline branch in `decodeBinaryNode` (or delegating the inline branch to it) so there is a single source of truth for frame decompression.</comment>
<file context>
@@ -1,308 +1,90 @@
- }
-
- return buffer
+export const decompressingIfRequired = async (buffer: Buffer) =>
+ 2 & buffer.readUInt8() ? await inflatePromise(buffer.subarray(1)) : buffer.subarray(1)
+
</file context>
8 participants
Stacked on #2067. Review that one first; this branch was rewritten on top of it and the diff here is only the WABinary codec.
Summary
WABinaryencode and decode move to the Rust bridge. The tree crosses the boundary once, as a flat buffer, rather than field by field: pulling a node apart from Rust throughReflectwas 56% of the encode profile, and returning a handle whose fields materialize on access moves the cost into the walk instead of removing it.A decode leaves its four sections (interned strings, string offsets, a u32 layout stream, blob bytes) where the decoder wrote them in linear memory and publishes their offsets to a fixed address. JavaScript reads them through cached whole-memory views, so a decode allocates no typed arrays at all. Byte content is the one thing that outlives the call, so that section is copied once and the leaves are views into the copy.
Measurement
Against
baileys@7.0.0-rc.9, the pure TypeScript codec this replaces. Both decoders warmed first, process pinned to one core, on the stanzas the socket actually moves:ack, 31 BThe warm-up matters and is worth flagging for anyone rerunning it: without it the first case measured absorbs the JIT warm-up for the whole file, about 150 ns, and it lands on whichever stanza happens to be first. That artifact read as the smallest stanza being the one case the bridge lost.
Memory is flat. The intern table that dedups strings within a decode is a fixed 512-slot direct-mapped table, so a socket that decodes two million stanzas with distinct message ids stays at 1.3 MB of linear memory. An earlier revision of this branch aged entries out by a round counter instead of removing them and reached 274 MB over the same run, which WASM never returns to the host.
Wire parity
Byte for byte against rc.9 on the recorded stanzas in
test/parity.test.tsandtest/server-response-parity.test.ts, plus a re-encode of every decode reproducing the original frame.Three behaviours were lost in an earlier revision and are restored here, each with a test:
s.whatsapp.netin the core, andhandleEncryptNotificationroutes onfrom === S_WHATSAPP_NET, so dropping the@sent the server's own pre-key count down the identity-change branch and replenishment never ran.nullorundefinedwere stringified onto the wire as the literal text. They reach the encoder unset from real call sites, a USync user queried by phone and a media retry with no participant among them.nulland crashed the encoder rather than being dropped.Also in this diff, and separable if you would rather
Two things found while profiling the send path that are not the codec. Say the word and I will move them to their own PR:
promiseTimeoutcapturednew Error().stackand so diddelayCancellable, which it calls. Capturing frames costs about 1.2 us; reading.stackmakes V8 format them and costs about 11 us. It was not confined to the timeout path either, because settling rancancel, which built a Boom and read its stack. Every outbound frame paid for two of those. Formatting is now deferred behind a getter andpromiseTimeoutgoes from 22.0 us to 3.0 us.generateParticipantHashV2ran on every send and was discarded:extraAttrshad already been spread into the<enc>nodes by the two awaitedcreateParticipantNodescalls above it. Fixing the order would not have helped, since the participant hash belongs on the message stanza rather than on<enc>, so it is removed rather than moved.Validation
The flat codec had no test of its own before this, only a bench. It gets one covering wire parity against a recorded stanza, each content kind, the token boundary, interning across decodes, and the two lifetime rules the borrowed sections depend on.
Merge order
This needs #2067 first, and #2067 needs
whatsapp-rust-bridgepublished with the flat exports: the tarball on npm today does not carrydecodeNodeFlat,encodeNodeFlatortokenTable, so a clean checkout cannot resolve them without building the bridge locally.The previous state of this branch is kept at
jlucaso1/Baileys@backup/pr-1698-pre-rewriteif anyone wants to read the earlier approach.Summary by CodeRabbit
Improvements
Bug Fixes
Tests