fix(iq): report <error> detail the IQ parser drops, once per process - #1202
Conversation
parse_iq_response reads code/text/type/backoff off an <error> node, which is exactly what WA Web's parseIqResponse keeps. What was never checked is whether the server sends more than that: a bare bad-request gives no way to tell an empty error apart from a detailed one the parser discarded. This adds a probe that names the unread parts (attribute names, child tags, and the kind of a raw payload) and nothing else, because a value can hold a JID and the report lands at a level production enables. It is a note to this library's maintainers rather than something a calling application can act on, and rejected IQs arrive in bursts, so it fires once per process. Both the once-flag and the log-level check run before the scan, so every further rejected IQ costs one relaxed load; the scan itself allocates nothing when there is nothing to report, and the function is #[cold] to keep it out of the receive path's code size. PARSED_ERROR_ATTRS mirrors what the parser reads, so reading a fifth attribute without adding it there is what would make the probe report a false positive.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe IQ response parser detects unparsed error-node content and emits a redacted warning once per process. Tests cover parsed errors, attributes, child nodes, byte/text payloads, and empty payloads. IQ error detail detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@wacore/src/request.rs`:
- Around line 549-561: The test a_fully_parsed_error_reports_nothing currently
relies on warn_on_dropped_error_detail reaching its warning branch, which
depends on global log::Level::Warn state. Update the test to assert the
warning-branch behavior directly through the relevant dropped-error-detail
result or branch-specific helper, while preserving the existing assertion that
fully parsed errors return None; do not rely on process-wide logger
configuration.
🪄 Autofix (Beta)
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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c12c3e1d-1932-4887-b897-0dbf4d4cd8e4
📒 Files selected for processing (1)
wacore/src/request.rs
The call sat in the fully-parsed-error test claiming to cover the caller's empty-detail return, but warn_on_dropped_error_detail exits at its log-level guard when no logger is installed, which is the case in this test binary. So it covered nothing, and with a logger installed it would instead flip the process-wide once-flag other tests share. The pure function's result is what the test can assert without depending on either.
Summary
RequestUtils::parse_iq_responsereadscode/text/type/backoffoff the<error>node and drops the rest. Those four are exactly what WA Web'sparseIqResponsekeeps, so reading only them is the right default, but "we read what WA Web reads" and "the server sent nothing more" are different claims and only the first was ever checked: a barebad-requestgives no way to tell an empty error from a detailed one the parser discarded.This adds a probe that names the unread parts and nothing else. It reports once per process, because the finding is a note to this library's maintainers rather than something a calling application can act on, and rejected IQs arrive in bursts (usync, prekey and app-state fan-outs all retry) where a per-occurrence warning would be noise at exactly the moment the log matters.
Protocol evidence
docs/captured-js/WA/Parse/IqResponse.jsbuilds its error result fromi.code,i.text,i.typeandi.backoffonly, confirming the four attributes we read are the full set WA Web keeps.typeof n == "function" ? (l = n(e)) : (l = n.parseOrThrow(a)), whereais the<error>node. So WA Web does have an escape hatch for a call site that needs more, exposed asdeprecatedSendIqErrorParserindocs/captured-js/WA/Deprecated/SendIq.js.Changes
dropped_error_detailnames the attributes outsidePARSED_ERROR_ATTRS, the child tags, and the kind of a raw payload. Names only, no values: an attribute value can hold a JID and the report lands at a level production enables. It readscontentdirectly rather thanchildren(), which answersNonefor a byte or string payload that is dropped just the same.PARSED_ERROR_ATTRSmirrors what the parser reads, with the parser commented to point at it. Reading a fifth attribute without adding it there is what would make the probe report a false positive on every response.static AtomicBooland bylog_enabled!, both checked before the scan: once the warning is out, or with warnings filtered off, every further rejected IQ costs one relaxed load. Racing callers settle it with aswap, so at most one message is emitted.warn_on_dropped_error_detailis#[cold]to keep it out ofparse_iq_response, which is on the receive path and has its code size gated in CI.generate_request_idnow uses the importedOrderinginstead of the full path, whichunused_qualificationsrequires once the import exists.Cost
The happy path is untouched: nothing here runs for a successful IQ. On a rejected one the scan walks the error node's attributes and its content discriminant, and allocates nothing when there is nothing to report (a
Filteryields a zero lower bound, so thecollectdoes not allocate). The twoVecs are only ever materialized on the single occurrence that logs.Not in this PR
If the probe does fire in production, the follow-up is to hand the detail to the caller rather than only to the log:
logis a facade, so an application that installs no logger sees nothing, and one that does could already raiseClient/Recvto debug. That means anIqError::ServerErrorfield carrying the node, which is a public error-surface change touching every construction site, and it is not worth making speculatively before the probe shows there is anything to carry.Validation
Full matrix left to CI.