Repository navigation
fix: browser ping timeout — gVisor MTU below IPv6 minimum - #8
Merged
Merged
Conversation
The browser stack used MTU 1000, below the IPv6 minimum link MTU. gVisor calculateNetworkMTU rejects every IPv6 packet with ErrInvalidEndpointState in that case, so zero datagrams ever left the browser while WebTransport registration succeeded, surfacing as 'ping: context deadline exceeded'. Small WT datagrams are still ensured by browserFragmentChunkSize. Also log relay drops for unknown destinations (was silent) and each failed browser ping attempt, and add a native regression test that rebuilds the browser-equivalent stack and asserts SYNs are emitted.
Reviewer's GuideFixes browser IPv6 packet transmission by increasing the gVisor MTU from 1000 to 1280, while preserving small WebTransport datagrams through fragmentation. The change centralizes the browser stack configuration, adds a native regression test confirming SYNs leave the stack, and improves relay and browser ping diagnostics. Sequence diagram for browser IPv6 ping through the MASQUE relaysequenceDiagram
participant Browser as Browser gVisor stack
participant WT as WebTransport
participant Relay as MASQUE relay
participant Dest as Destination
Browser->>Browser: calculateNetworkMTU(browserMasqueMTU)
Browser->>Browser: TCP SYN
Browser->>WT: Fragment into browserFragmentChunkSize datagrams
WT->>Relay: WebTransport datagrams
Relay->>Relay: lookup(pkt.dst)
Relay->>Dest: ForwardPacket(pkt.src, pkt.dst, pkt.payload)
Dest-->>Relay: Response packets
Relay-->>WT: WebTransport datagrams
WT-->>Browser: Reassembled response
Flow diagram for the browser MTU fixflowchart LR
A[browserMasqueMTU = 1280] --> B[IPv6 packet accepted by gVisor]
B --> C[browserFragmentChunkSize = 1000]
C --> D[Small WebTransport datagrams]
D --> E[Relay forwarding]
F[browserMasqueMTU = 1000] --> G[calculateNetworkMTU rejects IPv6 packet]
G --> H[No datagram leaves browser]
H --> I[Ping context deadline exceeded]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="masquecat_mtu_test.go">
<violation number="1" location="masquecat_mtu_test.go:69">
P3: The test hardcodes the ping target (`fd00:7461:696c:6361:7400:7069:6e67:1`, port 65535) instead of referencing `browserMasquePingAddr`/`browserMasquePingPort`, duplicating values that already exist in masquecat_browser_js.go. That contradicts the pattern this PR itself established in masquecat_mtu.go (whose comment says the constants live in the untagged file "so native regression tests can reference the exact value the browser build uses") — the ping address/port are just as bound into the browser stack. If the browser ping target ever changes, this test silently keeps testing a stale destination and can pass while the browser fails. Move `browserMasquePingAddr`/`browserMasquePingPort` to masquecat_mtu.go (or another untagged file) next to `browserMasqueMTU` and reference them here.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| t.Fatal(err) | ||
| } | ||
|
|
||
| pingAddr := netip.MustParseAddr("fd00:7461:696c:6361:7400:7069:6e67:1") |
There was a problem hiding this comment.
P3: The test hardcodes the ping target (fd00:7461:696c:6361:7400:7069:6e67:1, port 65535) instead of referencing browserMasquePingAddr/browserMasquePingPort, duplicating values that already exist in masquecat_browser_js.go. That contradicts the pattern this PR itself established in masquecat_mtu.go (whose comment says the constants live in the untagged file "so native regression tests can reference the exact value the browser build uses") — the ping address/port are just as bound into the browser stack. If the browser ping target ever changes, this test silently keeps testing a stale destination and can pass while the browser fails. Move browserMasquePingAddr/browserMasquePingPort to masquecat_mtu.go (or another untagged file) next to browserMasqueMTU and reference them here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At masquecat_mtu_test.go, line 69:
<comment>The test hardcodes the ping target (`fd00:7461:696c:6361:7400:7069:6e67:1`, port 65535) instead of referencing `browserMasquePingAddr`/`browserMasquePingPort`, duplicating values that already exist in masquecat_browser_js.go. That contradicts the pattern this PR itself established in masquecat_mtu.go (whose comment says the constants live in the untagged file "so native regression tests can reference the exact value the browser build uses") — the ping address/port are just as bound into the browser stack. If the browser ping target ever changes, this test silently keeps testing a stale destination and can pass while the browser fails. Move `browserMasquePingAddr`/`browserMasquePingPort` to masquecat_mtu.go (or another untagged file) next to `browserMasqueMTU` and reference them here.</comment>
<file context>
@@ -0,0 +1,90 @@
+ t.Fatal(err)
+ }
+
+ pingAddr := netip.MustParseAddr("fd00:7461:696c:6361:7400:7069:6e67:1")
+ ctx, cancel := context.WithTimeout(context.Background(), 4*time.Second)
+ defer cancel()
</file context>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
Browser demo at https://knowlet.github.io/MasqueCat/ failed every dial with
ping: context deadline exceeded, while CLI (native) through the same relay worked (pong via relay-masque).Root cause
The browser (js/wasm) gVisor stack used MTU 1000, below the IPv6 minimum link MTU of 1280 (RFC 8200). gVisor calculateNetworkMTU rejects every outbound IPv6 packet with ErrInvalidEndpointState in that case.
Evidence from an instrumented wasm build:
Fix
Also included
Verification
Summary by Sourcery
Restore browser connectivity by using an IPv6-compliant MTU while preserving small WebTransport datagram fragmentation.
Bug Fixes:
Tests:
Summary by cubic
Fixes the browser (js/wasm) demo dial failures where
pingalways timed out, even though the native CLI worked through the same relay. The browser gVisor stack used an MTU of 1000, below the IPv6 minimum link MTU of 1280 (RFC 8200), causing gVisor to reject every outbound IPv6 packet and never send a datagram.Bug Fixes
browserFragmentChunkSize.Written for commit 81e1539. Summary will update on new commits.