twiddle: support UDP tunneling through UoT - #321
Conversation
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughTwiddle now supports UDP over TCP through UoT. The inbound wraps its router for UoT-aware handling. End-to-end tests cover TCP, UDP, and packet-mode traffic. The Makefile runs tests with the default Go configuration. ChangesTwiddle UoT transport
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Twiddle now carries connected and packet-mode UDP traffic over its existing TCP transport while retaining TCP behavior. The supplied coverage exercises the new routing paths, with no remaining merge-blocking risk identified. Sequence Diagram(s)sequenceDiagram
participant Caller
participant TwiddleOutbound
participant UoTClient
participant TwiddleInbound
participant EchoRouter
Caller->>TwiddleOutbound: Dial or listen for TCP/UDP traffic
TwiddleOutbound->>UoTClient: Route UDP through UoT
UoTClient->>TwiddleInbound: Carry traffic over TCP
TwiddleInbound->>EchoRouter: Route connection or packet
EchoRouter-->>Caller: Echo payload and routing metadata
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR implements the UDP/UoT adapter changes. However, linked issue Resolution Fix the Twiddle core race before merge. Serialize close-time and data writes through the existing write lock, or make writeSized respect the flushing state. Add or retain a race test that verifies safe sequencing of sendSeq and AEAD record writes. Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The new test router embeds a nil adapter.Router, which can panic at runtime if non-Ex router methods are invoked (particularly via the UoT router wrapper).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds UDP tunneling support to the Twiddle protocol adapter by using sing-box’s UDP-over-TCP (UoT v2) path, aligning Twiddle’s capabilities with other UoT-enabled outbounds (e.g., Samizdat), and updates local tooling/tests accordingly.
Changes:
- Advertise UDP support in the Twiddle outbound and implement UDP via
uot.ClientforDialContextandListenPacket. - Wrap the Twiddle inbound router with sing-box’s UoT router to enable packet dispatch for UoT connections.
- Add a loopback integration test covering TCP, connected UDP, and multi-destination packet routing; fix
make testtarget behavior.
File summaries
| File | Description |
|---|---|
| protocol/twiddle/twiddle_test.go | Adds integration test that exercises Twiddle TCP + UDP-over-TCP paths and router dispatch. |
| protocol/twiddle/outbound.go | Enables UDP on outbound and routes UDP + packet APIs through uot.Client. |
| protocol/twiddle/inbound.go | Wraps router with UoT router and updates inbound router type to support packet routing. |
| Makefile | Declares test as phony and removes obsolete GOEXPERIMENT=synctest. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… half uot.Client was wired with the Outbound as its own Dialer, which made DialContext(udp) and ListenPacket self-referential. Neither recurses against sing v0.8.13 -- uot.Client dials TCP for the tunnel and never calls Dialer.ListenPacket -- but both of those are upstream details rather than properties of this package, and if a bump changes either the symptom is an unbounded recursion in production, not a compile error. uotDialer opens the outer tunnel and refuses anything else, which is how unbounded, samizdat and water each wire theirs. dialTunnel is the shared body, so the TCP path and UoT's inner dial cannot drift. echoRouter left RouteConnection and RoutePacketConnection on a nil embedded adapter.Router. uot.Router.RouteConnection forwards to the wrapped router's RoutePacketConnection -- the non-Ex one -- so that path was a nil-interface panic waiting for whatever first took it. Only the Ex path runs today, which is why the test passed. Both are defined now, matching the mockRouters in samizdat and meek. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gn6KuHUL766qQNn8m1fu1
There was a problem hiding this comment.
🟢 Approval recommended
The implementation follows established in-repo UoT patterns and includes an integration test that exercises the new TCP/UDP/packet flows end-to-end.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
getlantern/twiddle#2 is merged. Conn.writeRecord now holds a dedicated mutex across the seal, the sequence increment and the socket write, which it did not before: Write releases wmu around each record so a slow socket cannot block other writers, and Close reached writeRecord through writeSized on that freed lock. Two sealers could take one sendSeq, and the sequence number is the AEAD nonce, so that was nonce reuse under a single key rather than a decrypt failure -- and records could reach the wire out of the order they were sealed in, which the peer's monotonic counter cannot authenticate either. It matters here because UoT gives a twiddle Conn the ordinary sing-box shape of a bidirectional copy: one direction writing while the other closes on error is exactly the overlap involved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gn6KuHUL766qQNn8m1fu1
There was a problem hiding this comment.
🟢 Approval recommended
The implementation appears complete and is backed by a new end-to-end test; the only feedback is a minor test robustness nit.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
protocol/twiddle/twiddle_test.go:100
- In the accept goroutine, conn.SetDeadline(...) ignores its returned error. If SetDeadline fails (e.g., unusual net.Conn implementations), the test can hang longer than intended or behave inconsistently; it’s safer to handle the error and abort the goroutine early.
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
#321 made dialTunnel mean "open one outer twiddle connection" and pointed UoT's inner dial at it. This branch replaces that model with one session and a stream per dial, so the two had to be reconciled rather than merged textually: taken naively, every UDP association would have opened a twiddle tunnel of its own, putting a fresh TLS-shaped opening on the wire for every DNS lookup. That is the pattern muxing exists to remove. dialTunnel now means "open a stream on the shared session", and uotDialer still points at it, so UDP rides the mux with everything else. Network() keeps both protocols, Close() keeps the session teardown, and the GO_AWAY handling moves with the retry loop into dialTunnel. Two things in #321's test assumed the pre-mux lifetime. NewConnectionEx blocked there for the life of the connection; here it hands the session to a goroutine and returns, so the test's `defer conn.Close()` tore the tunnel down before the client could open a stream, and its SetDeadline bounded the whole session rather than one connection. The test now holds the tunnel open explicitly. And echoRouter called onClose unconditionally, which routeStream's nil argument turned into a panic. nil is legitimate: sing-box's own router funnels onClose through N.CloseOnHandshakeFailure, which tolerates it, and a stream has no per-stream close bookkeeping to do. The double now guards, like the production path does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gn6KuHUL766qQNn8m1fu1
Summary
Add UDP tunneling to Twiddle using the same sing-box UDP-over-TCP (UoT v2) path as Samizdat.
DialContextand multi-destination UDP throughListenPacket.make test: declare the target phony (the existingtest/directory made CI skip it), and remove the obsoleteGOEXPERIMENT=synctestflag rejected by the module's Go 1.25 toolchain. Existing experiment-tagged legacy tests are not migrated in this PR.Flow
sequenceDiagram autonumber participant C as Caller participant O as Outbound<br/>outbound.go participant I as Inbound<br/>inbound.go participant R as UoT router C->>O: outbound.go:183 / 256<br/>DialContext(udp) or ListenPacket Note over O: outbound.go:176<br/>UoT uses this outbound's TCP path ⚠️ O->>I: outbound.go:199<br/>authenticated Twiddle session over TCP O->>I: outbound.go:212<br/>reserved UoT destination, then request and datagrams rect rgba(255, 200, 200, 0.3) Note over I: inbound.go:83<br/>UoT wrapper fixes missing packet dispatch 🐛 end I->>R: inbound.go:142<br/>RouteConnectionEx recognizes UoT destination R-->>C: inbound.go:83<br/>packet routing preserves datagram boundaries and destinationsVerification
go test -race -count=20 -timeout=3m ./protocol/twiddlepassed.go test ./...with CI's e2e build tags passed.go vetfor the Twiddle package, including the full tag set, passed.make testpasses locally.Rollout / scope
Both client and egress need this adapter update for UDP. No dependency bump, new configuration option, or UDP listening port is required.
This PR does not revive closed #319 or fix the separate credential-reuse/concurrency issue documented in getlantern/lantern-cloud#3291. Full-handshake profile activation is also unchanged.
Summary by CodeRabbit
New Features
Bug Fixes
Tests