fix(btest): UDP receive 0 bps — remove connect() filter, warn on silent --remote-udp-tx-size - #86
Conversation
…p-tx-size UDP receive was returning 0 bps for direction=receive and direction=both because udp.connect() installs a BSD kernel receive filter that silently drops datagrams whose source port does not exactly match the connected peer. RouterOS may use a separate TX socket (different source port than the negotiated serverUdpPort), so the filter discarded every incoming packet. Fix: remove the connect() call on the client UDP socket and mirror the server-side pattern of addressing sends explicitly via udpTxLoop's target argument. The initial empty datagram (NAT/SLIRP flow-open) is also updated to carry the explicit server address. The socket now accepts datagrams from any source on clientPort, consistent with how the server side already works. Also add a warning to the client envelope when --remote-udp-tx-size and --local-udp-tx-size differ for --direction both: the btest wire protocol carries a single tx-size field so remoteUdpTxSize is silently discarded in that combination. Tracking: --connection-count not wired to handshake (#84); TCP both client has no status reader starving server→client RX (#85). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oot cause RouterOS sends UDP data from a source port different from the negotiated serverUdpPort (separate TX socket). The BSD connect() receive filter on the client socket silently dropped every incoming datagram. Fixed by removing connect() and addressing sends explicitly (same as the server side). Real-device validation against a live RouterOS instance: direction=receive 163-203 Mbps, direction=both 104 Mbps RX / 83 Mbps TX. CI integration test still covers TCP only (SLIRP blocks the UDP reverse path). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml 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:
📝 WalkthroughWalkthroughRemoves ChangesUDP Socket Fix, Warning Collection, and Docs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Fixes btest UDP client receive reporting (rx 0 bps) by removing the client-side UDP connect() receive filter and switching the client to explicit per-datagram addressing, plus adds a user-facing warning for an otherwise silently ignored option combination.
Changes:
- Remove UDP
connect()from the btest client and send the initial “flow open” datagram with an explicit(host, port). - Update UDP session driving to compute an explicit target for both client and server sends.
- Add a warning when
--remote-udp-tx-sizeis overridden/ignored for--direction both, and document the root cause/evidence indocs/MATRIX.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/protocols/btest-session.ts | Removes UDP connect() on the client and routes UDP sends through explicit addressing in the session driver. |
| src/btest.ts | Adds a warning surfaced in the envelope when --remote-udp-tx-size is effectively ignored for --direction both. |
| docs/MATRIX.md | Updates matrix notes with the root cause and real-device validation evidence for the UDP fix. |
| warnings.push({ | ||
| code: "validation/option", | ||
| message: `--remote-udp-tx-size (${request.remoteUdpTxSize}) is ignored for --direction both; the btest wire protocol carries a single tx-size and --local-udp-tx-size (${request.localUdpTxSize}) is used for both directions.`, | ||
| }); |
| const target = | ||
| role === "server" && | ||
| ctx.clientUdpPort !== undefined && | ||
| ctx.udpPeerHost !== undefined | ||
| ? { port: ctx.clientUdpPort, host: ctx.udpPeerHost } | ||
| : undefined; | ||
| : role === "client" && | ||
| ctx.serverUdpPort !== undefined && | ||
| ctx.udpPeerHost !== undefined | ||
| ? { port: ctx.serverUdpPort, host: ctx.udpPeerHost } | ||
| : undefined; |
| : role === "client" && | ||
| ctx.serverUdpPort !== undefined && | ||
| ctx.udpPeerHost !== undefined | ||
| ? { port: ctx.serverUdpPort, host: ctx.udpPeerHost } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/protocols/btest-session.ts (1)
1448-1458: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFail fast if UDP TX target is unresolved.
If
dirs.shouldTxis true andtargetresolves toundefined,udpTxLoopcan degrade into retried send failures instead of a clear operator-facing error.As per coding guidelines, "Errors must be actionable for humans and agents, with next-step guidance when a dependency, protocol, credential, or validation source is missing."
Proposed guard
const target = role === "server" && ctx.clientUdpPort !== undefined && ctx.udpPeerHost !== undefined ? { port: ctx.clientUdpPort, host: ctx.udpPeerHost } : role === "client" && ctx.serverUdpPort !== undefined && ctx.udpPeerHost !== undefined ? { port: ctx.serverUdpPort, host: ctx.udpPeerHost } : undefined; + if (dirs.shouldTx && target === undefined) { + throw new Error( + `UDP transmit target unresolved for ${role}; check negotiated UDP peer port/host.`, + ); + } if (dirs.shouldTx) tasks.push(udpTxLoop(ctx, target));🤖 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 `@src/protocols/btest-session.ts` around lines 1448 - 1458, Add a guard check immediately after the `target` variable is assigned that verifies if `dirs.shouldTx` is true, then `target` must not be undefined. If this condition fails, throw an actionable error with a clear message explaining which required UDP configuration (either client/server UDP port or UDP peer host) is missing, before attempting to push `udpTxLoop` into the tasks array. This prevents the function from silently passing undefined to `udpTxLoop` which would result in confusing retry failures.Source: Coding guidelines
🤖 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 `@src/protocols/btest-session.ts`:
- Around line 1227-1243: The udp.send() call on line 1238 can throw an
exception, which will leave the udp socket and channel open and prevent the
normal close path from executing. Wrap the udp.send() call in a try-catch block,
and in the catch handler, ensure that both the udp socket and the channel are
properly closed before re-throwing or handling the error appropriately to
guarantee cleanup even when UDP bootstrap fails.
---
Nitpick comments:
In `@src/protocols/btest-session.ts`:
- Around line 1448-1458: Add a guard check immediately after the `target`
variable is assigned that verifies if `dirs.shouldTx` is true, then `target`
must not be undefined. If this condition fails, throw an actionable error with a
clear message explaining which required UDP configuration (either client/server
UDP port or UDP peer host) is missing, before attempting to push `udpTxLoop`
into the tasks array. This prevents the function from silently passing undefined
to `udpTxLoop` which would result in confusing retry failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9b9e963-3477-43e6-ae2e-afac7a2a573c
📒 Files selected for processing (3)
docs/MATRIX.mdsrc/btest.tssrc/protocols/btest-session.ts
| udp = (options.createUdpSocket ?? createBtestUdpSocket)(); | ||
| await udp.bind(negotiated.clientUdpPort as number, "0.0.0.0"); | ||
| await udp.connect(negotiated.serverUdpPort as number, options.host); | ||
| // Do NOT connect() the socket: connect() installs a kernel receive filter | ||
| // that silently drops datagrams whose source port differs from the connected | ||
| // peer. RouterOS may send UDP data from a different source port than the | ||
| // negotiated serverUdpPort (separate TX socket), so the filter would discard | ||
| // all incoming packets and leave rx at 0 bps throughout. Instead, keep the | ||
| // socket unconnected (like the server side) and always address sends | ||
| // explicitly via udpTxLoop's target argument. | ||
| if (options.natMode || dirs.shouldRx) { | ||
| // Originate a flow so the server's datagrams can return (NAT/SLIRP). | ||
| udp.send(new Uint8Array(0)); | ||
| udp.send( | ||
| new Uint8Array(0), | ||
| negotiated.serverUdpPort as number, | ||
| options.host, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close sockets/channel on UDP bootstrap failure.
On Line 1238, udp.send(...) can throw before the normal close path, leaving udp/channel open on failure.
Proposed fix
let udp: BtestUdpSocket | undefined;
if (options.protocol === "udp") {
udp = (options.createUdpSocket ?? createBtestUdpSocket)();
- await udp.bind(negotiated.clientUdpPort as number, "0.0.0.0");
- // Do NOT connect() the socket: connect() installs a kernel receive filter
- // ...
- if (options.natMode || dirs.shouldRx) {
- udp.send(
- new Uint8Array(0),
- negotiated.serverUdpPort as number,
- options.host,
- );
- }
+ try {
+ await udp.bind(negotiated.clientUdpPort as number, "0.0.0.0");
+ // Do NOT connect() the socket: connect() installs a kernel receive filter
+ // ...
+ if (options.natMode || dirs.shouldRx) {
+ udp.send(
+ new Uint8Array(0),
+ negotiated.serverUdpPort as number,
+ options.host,
+ );
+ }
+ } catch (error) {
+ udp.close();
+ channel.close();
+ throw error;
+ }
}🤖 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 `@src/protocols/btest-session.ts` around lines 1227 - 1243, The udp.send() call
on line 1238 can throw an exception, which will leave the udp socket and channel
open and prevent the normal close path from executing. Wrap the udp.send() call
in a try-catch block, and in the catch handler, ensure that both the udp socket
and the channel are properly closed before re-throwing or handling the error
appropriately to guarantee cleanup even when UDP bootstrap fails.
…t cleanup, ternary Code-review fixes on the UDP-receive change: - Warning code: the new --remote-udp-tx-size advisory used `validation/option`, which is an *error* code (unknown/invalid value). Switch to a btest-specific warning `routeros/btest-udp-tx-size-ignored` (+ structured context), catalog it, and add its docs/errors page. (Copilot) - UDP hot-path DNS: now that the client addresses every datagram explicitly (no connect()), passing a hostname would make node:dgram run a DNS lookup per send. Reuse the IP already resolved for the TCP control connection (channel.remoteAddress) as udpPeerHost — mirrors the server side; no extra lookup. (Copilot) - Socket cleanup: wrap the UDP bind/flow-open bootstrap in try/catch so a bind()/send() throw closes the socket and control channel instead of leaking them. (CodeRabbit) - Readability: replace the nested ternary computing the udpTxLoop target with an explicit if/else. (Copilot) Adds unit tests asserting the warning fires (and its context) for UDP --direction both with differing tx-sizes, and does not fire when they match. Tracking sweep so undone btest items stop getting lost: - Reopened #69 (Windows UDP-loopback tier still skips, not green; #86 adds another skipped test) - Filed #87 (TCP --connection-count > 1 parallel fan-out, the follow-up #84 promised was tracked separately) - Filed #88 (UDP client→server has no CI coverage; SLIRP blocks the reverse path, validated manually only) - MATRIX caveats now point at #84/#87/#88/#69 bun run lint && bun run test (768 pass, 28 skip, 0 fail) && bun run build green; lint:ci green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed Review items
Added unit tests asserting the warning fires (with its context) for UDP Tracking sweep (so undone btest items stop getting lost):
🤖 Generated with Claude Code |
…erse-path gap #88 asked for CI coverage of the btest client's UDP path (validated manually only). Research outcome: **no quickchr change needed.** The user's socket-connect L2-bridge idea would work but is unnecessary — a CHR probe showed the server→client UDP return already lands over the guest→host SLIRP **gateway** (`10.0.2.2:clientUdpPort`, the same path the server cell's UDP-transmit uses), with only the existing TCP control forward. It works because PR #86 left the client UDP socket unconnected (a `connect()` filter previously dropped every datagram); the gap was simply that this was never CI-tested. - `btest-client.test.ts`: add UDP `receive` (rx > 0) and `both` (tx > 0 && rx > 0) cycles against real CHR `/tool/bandwidth-server`. CHR 7.23.1: receive rx≈474KB, both tx>0 rx≈582KB with UDP loss accounting. The reverse path (rx) is asserted; client→server transmit verification needs server-side stats and stays covered by the server cell (symmetric guest→host). - Docs: README (validation policy, honest-grounding, open questions, out-of-scope), MATRIX caveat, examples client-cell bullet — UDP client receive/both now gated; the one remaining unproven UDP edge is the server cell's host→guest direction (would need a UDP hostfwd). Closes #88 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erse-path gap #88 asked for CI coverage of the btest client's UDP path (validated manually only). Research outcome: **no quickchr change needed.** The user's socket-connect L2-bridge idea would work but is unnecessary — a CHR probe showed the server→client UDP return already lands over the guest→host SLIRP **gateway** (`10.0.2.2:clientUdpPort`, the same path the server cell's UDP-transmit uses), with only the existing TCP control forward. It works because PR #86 left the client UDP socket unconnected (a `connect()` filter previously dropped every datagram); the gap was simply that this was never CI-tested. - `btest-client.test.ts`: add UDP `receive` (rx > 0) and `both` (tx > 0 && rx > 0) cycles against real CHR `/tool/bandwidth-server`. CHR 7.23.1: receive rx≈474KB, both tx>0 rx≈582KB with UDP loss accounting. The reverse path (rx) is asserted; client→server transmit verification needs server-side stats and stays covered by the server cell (symmetric guest→host). - Docs: README (validation policy, honest-grounding, open questions, out-of-scope), MATRIX caveat, examples client-cell bullet — UDP client receive/both now gated; the one remaining unproven UDP edge is the server cell's host→guest direction (would need a UDP hostfwd). Closes #88 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erse-path gap #88 asked for CI coverage of the btest client's UDP path (validated manually only). Research outcome: **no quickchr change needed.** The user's socket-connect L2-bridge idea would work but is unnecessary — a CHR probe showed the server→client UDP return already lands over the guest→host SLIRP **gateway** (`10.0.2.2:clientUdpPort`, the same path the server cell's UDP-transmit uses), with only the existing TCP control forward. It works because PR #86 left the client UDP socket unconnected (a `connect()` filter previously dropped every datagram); the gap was simply that this was never CI-tested. - `btest-client.test.ts`: add UDP `receive` (rx > 0) and `both` (tx > 0 && rx > 0) cycles against real CHR `/tool/bandwidth-server`. CHR 7.23.1: receive rx≈474KB, both tx>0 rx≈582KB with UDP loss accounting. The reverse path (rx) is asserted; client→server transmit verification needs server-side stats and stays covered by the server cell (symmetric guest→host). - Docs: README (validation policy, honest-grounding, open questions, out-of-scope), MATRIX caveat, examples client-cell bullet — UDP client receive/both now gated; the one remaining unproven UDP edge is the server cell's host→guest direction (would need a UDP hostfwd). Closes #88 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…CHR gate (#88) (#99) * feat(btest): TCP multi-connection fan-out (#87) — client opens negotiated secondaries centrs's btest client now realizes `--connection-count > 1`: it reads the session token from the primary's OK and opens connection-count-1 additional TCP data connections, driving them into one shared BandwidthCounters so throughput aggregates. Closes the #87 fan-out (#84 wired the flag; this moves the data). Wire format grounded byte-for-byte against real RouterOS 7.23.1 (probed via CHR, all directions): - Server OK carries the session token in bytes 1-2 BE; RouterOS generates a non-zero token for connection-count > 1. - Each secondary sends a 16-byte join `[token:u16 BE][0x02][0 …]` — byte[2]=0x02 is a constant, direction-independent marker (encodeSecondaryJoin was missing it). - The server sends a 4-byte HELLO per connection, then **no** ack before data — it waits for all connections to join before streaming. So the join must not block on a reply read, or the sequential opens deadlock (this was the first implementation's bug, caught on CHR: secondary#1 stalled, server timed out and dropped it). Confirmed: a CHR run opens all 4 connections and data flows on each. Behavior: - Unauthenticated TCP fan-out works end to end. Authenticated (EC-SRP5) sessions stay single-stream (the post-auth token is not captured) and warn when the realized `activeConnections` falls short of the request. The pre-session "single stream" warning is replaced by this accurate post-session one. - `BtestRunSummary`/`BtestClientData` gain `activeConnections` (the realized count). Tests: - Unit: secondary-join byte format (`abcd0200…`); loopback fan-out opens N connections, sends the grounded join, and drives data on all N; full-fan-out and short-fan-out warning cases. - Integration (CHR 7.23.1): `btest-client.test.ts` example 11 — centrs client `connection-count=4` → real `/tool/bandwidth-server` opens 4 connections (`activeConnections == 4`), data flows. No throughput-rise assertion: the near-zero-latency SLIRP loopback is bandwidth-bound, so multi-connection does not raise aggregate throughput there (a WAN/latency property); the per-connection drive is asserted deterministically by the unit test. Docs: README (status, how-it-works, flag, honest-grounding, open questions), examples.md (example 5 clarified as server-side accept; new example 11), MATRIX caveat, error catalog + page, module header. Closes #87 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(btest): gate UDP client receive/both on CHR — closes the #88 reverse-path gap #88 asked for CI coverage of the btest client's UDP path (validated manually only). Research outcome: **no quickchr change needed.** The user's socket-connect L2-bridge idea would work but is unnecessary — a CHR probe showed the server→client UDP return already lands over the guest→host SLIRP **gateway** (`10.0.2.2:clientUdpPort`, the same path the server cell's UDP-transmit uses), with only the existing TCP control forward. It works because PR #86 left the client UDP socket unconnected (a `connect()` filter previously dropped every datagram); the gap was simply that this was never CI-tested. - `btest-client.test.ts`: add UDP `receive` (rx > 0) and `both` (tx > 0 && rx > 0) cycles against real CHR `/tool/bandwidth-server`. CHR 7.23.1: receive rx≈474KB, both tx>0 rx≈582KB with UDP loss accounting. The reverse path (rx) is asserted; client→server transmit verification needs server-side stats and stays covered by the server cell (symmetric guest→host). - Docs: README (validation policy, honest-grounding, open questions, out-of-scope), MATRIX caveat, examples client-cell bullet — UDP client receive/both now gated; the one remaining unproven UDP edge is the server cell's host→guest direction (would need a UDP hostfwd). Closes #88 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(btest): fix review nits — secondary join sends no pre-data ack; MATRIX fan-out wording Copilot/CodeRabbit on #99: - Module header + encodeSecondaryJoin comment no longer imply a "server OK" on the secondary join — RouterOS sends no acknowledgement before bulk data (the implementation deliberately does not read one). - MATRIX: the fan-out is CHR-gated for the realized connection count (activeConnections == count), not a throughput rise (SLIRP loopback is bandwidth-bound) — wording now matches what the test asserts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(btest): clarify example 5 is the server-side accept, distinct from client fan-out (example 11) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(btest): address CodeRabbit #99 — pin secondaries to primary IP, finally-cleanup, narrow UDP caveat - runBtestClientSession: open secondaries at `channel.remoteAddress ?? options.host` (the IP the primary control socket resolved to), so a load-balancing hostname can't land a secondary on a different RouterOS where the token is unknown. - Wrap the primary+secondary `Promise.all` in try/finally so a rejected loop still closes the UDP socket and every TCP channel (no leak on the error path). - README Open questions: the stale "UDP receive/both through SLIRP is unproven" bullet contradicted the now-gated client cell — narrow it to the one open edge (server-cell host→guest). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(btest): link follow-up issues #100 (server-side accept) and #103 (auth multi-conn) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(matrix): clarify the tcp:2000 forward is TCP while UDP rides the gateway (no contradiction) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Replace the hand-rolled `outDir.replace(/\/+$/, "")` trailing-slash trim (CodeQL js/polynomial-redos #86) and the manual basename/extension helpers with node:path `join`/`basename`/`extname` — no regex, correct path handling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Final phase of the fan-out first-class plan: `centrs transfer` adopts the shared selection grammar via `src/transfer-fanout.ts`, completing uniform fan-out across api/retrieve/execute/transfer. CLI boundary = verb keyword; `download` fan-out requires `--out-dir` (collision-safe per-target file named by CDB identity); upload/remove/mkdir/copy are `--yes`-gated once up front. Also: `terminal` now rejects a selection with `usage/fanout-not-supported` (doc-vs-code alignment); README fan-out overview broadened. CHR-passed 7.23.1 (transfer-fanout F1–F6). CodeQL #86 (ReDoS) fixed via node:path, #87 dismissed false-positive; CodeRabbit items addressed (the --yes-gate one declined on merit — TTY confirmation matches api/execute).
Summary
udp.connect()from the client UDP socket — the BSD/Winsockconnect()call installs a kernel-level receive filter that silently drops datagrams whose source address does not match the connected peer exactly. RouterOS sends UDP data from a different source port than the negotiatedserverUdpPort(it uses a separate TX socket), so every incoming datagram was dropped, leavingrx 0 bpsfor--direction receiveand--direction boththroughout a session.send_towith explicit target). The initial flow-open datagram also gets an explicit address so the socket stays unconnected end-to-end.--remote-udp-tx-sizeis silently discarded for--direction both— the btest wire protocol carries a singletx_sizefield;localUdpTxSizewins forbothand the remote value was previously ignored without feedback.docs/MATRIX.md— add real-device evidence and document the root cause.Root cause (confirmed by live experiment)
Session query against a real RouterOS device mid-test showed
tx-current=0on the bandwidth-server session even though direction wassend. Switching to the local fixed build immediately yielded 163–203 Mbps receive and 104 Mbps bidirectional UDP. The oldconnect()filter was dropping RouterOS's datagrams because they arrived from a source port different fromserverUdpPort.Cross-platform
The fix uses
socket.send(bytes, port, host)(explicitsendto()) on an unbound socket — the standard path for all platforms. No platform-specific code added. UDP loopback unit tests (UDP both,EC-SRP5 over UDP receive) exercise the new explicit-target path on Linux and macOS; they skip on Windows due to the pre-existingSO_REUSEPORT/ENOTSUPprobe (issue #69, unrelated).Test plan
bun run lint && bun run test && bun run build— 766 pass, 28 skip, 0 fail--direction receive163–203 Mbps per interval,--direction both104 Mbps RX + 83 Mbps TXUDP both,EC-SRP5 over UDP receive) — these now exercise the explicit-target client pathcross-platform-unitTracking
Open issues filed during analysis:
--connection-countparsed but not wired to command packet (silent no-op)direction=bothclient has no status reader; TX dominates, starving server→client RX🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
New Features