Pre-tinygo optimizations and codebase cleanups#113
Conversation
- Add //go:build !prod tags to main_js.go and inject.go - Create main_js_prod.go with //go:build prod tag - Remove zerolog dependency from production build - Make InjectHTML a no-op in production (handled by service worker) This reduces WASM binary size and runtime overhead by excluding debug logging and HTML parsing from production builds.
- Add -gcflags="-l=4 -trimpath" for aggressive inlining - Add -ldflags="-s -w -buildid=" to strip symbols and build ID - Add -tags=prod to enable production-only code path - Implement dual-pass wasm-opt: -O4 for speed, then -Oz for size - Enable bulk-memory, sign-ext, and reference-types features This prioritizes runtime performance over binary size.
The WebSocket dialer returns a response that was never closed, causing a resource leak. Now closes the response body on error.
Replace dynamic slice growth with pre-allocation using make() with known capacity. This reduces allocations during relay iteration.
Change sync.Pool to store *[]byte instead of []byte to prevent interface boxing allocation on each Get/Put operation. Applied to: - cmd/portal-tunnel/main.go - cmd/relay-server/manager/bps_manager.go - portal/utils/ratelimit/bucket.go
…y' instead of 'proto.Message' - Changed the return type of CloneMessageVT methods in various structs from proto.Message to any. - Updated the parameter type of EqualMessageVT methods to accept 'any' instead of 'proto.Message'. - This change enhances type flexibility and aligns with recent Go language updates.
- Add rdsec_test.go covering all 4 message types (Identity, ClientInitPayload, SignedPayload, ServerInitPayload) - Round-trip MarshalVT/UnmarshalVT tests - CloneVT, EqualVT, SizeVT tests - Getter methods, Reset methods, and enum String/Enum tests - Coverage: 52.5% - Add rdverb_test.go covering all 11 message types (Packet, RelayInfo, Lease, etc.) - All packet type and response code enum tests - Getter methods and Reset methods - Complex nested message tests - Coverage: 36.6% - Add wsstream_test.go with comprehensive WebSocket stream wrapper tests - Mock implementation for isolated testing - Concurrent read/write safety tests - Multi-message streaming tests - Coverage: 93.1% - Refactor wsstream to use webSocketConn interface for testability - Update utils/ws.go to use wsstream.New() constructor
- Updated test cases in various files to use range-based loops for better readability and performance. - Modified the `justfile` to include `tidy` in the `all` target. - Enhanced `pre-commit-config.yaml` to ensure proper linting and formatting for Go files. - Added comprehensive tests for rate limiting and WebSocket functionalities in `utils` and `wsstream`. - Improved error handling and edge case coverage in existing tests.
Fix flakiness and incorrect assumptions in bucket tests:
- Trigger lazy state update explicitly in slack refill test
- Adjust burst/rate ratios to reliably trigger throttling in concurrent tests
- Increase rate for buffer pool test to avoid timeouts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Current message exhausted, try to get next one | ||
| g.currentReader = nil | ||
| continue | ||
| } |
There was a problem hiding this comment.
EOF handling drops websocket payload bytes
High Severity
WsStream.Read now loops on io.EOF and immediately retries, which discards n bytes when a reader returns data together with EOF. The same pattern also converts close errors to io.EOF with 0 bytes. This can silently truncate data read from websocket frames.
Additional Locations (1)
| s, ok := PacketType_name[int32(x)] | ||
| if !ok { | ||
| return "PacketType(" + string(rune('0'+x)) + ")" | ||
| } |
There was a problem hiding this comment.
Unknown enum string formatting is incorrect
Low Severity
For unknown enum values, String() now builds text using string(rune('0'+x)), which does not produce decimal numbers and can emit unrelated Unicode characters. This breaks readable fallback formatting for forward-compatible or unexpected enum values.
Additional Locations (2)
| "error": err.Error(), | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Failed upgrade leaves stale SDK connection
Medium Severity
After storing conn in sdkConnections and sending SDK_CONNECT_SUCCESS, upgrade validation/write failures return early without removing the map entry or closing the connection. This leaves orphaned live connections and inconsistent client state for failed upgradeRequest paths.
Additional Locations (1)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces production WASM proxy functionality with SDK bridging, refactors buffer pools across multiple modules for reduced allocations, migrates protocol serialization to VT-based marshalling, adds extensive test coverage for protocol and utility functions, and updates build configuration with production-oriented WASM optimization and justfile automation. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript Layer
participant Proxy as Production Proxy
participant DNS as DNS/Lease Cache
participant SDK as SDK Dialer
participant Upstream as Upstream Server
JS->>Proxy: HTTP/WS Request via __go_jshttp
Proxy->>Proxy: Parse request & route
alt WebSocket Polyfill
Proxy->>Proxy: Extract or create connection ID
Proxy->>DNS: Lookup lease ID for hostname
DNS->>Proxy: Return lease ID
Proxy->>SDK: Create/dial connection to upstream
SDK->>Upstream: Establish connection
Upstream->>SDK: Connection ready
SDK->>Proxy: Connection established
Proxy->>Proxy: Queue connection in WebSocketManager
Proxy->>JS: Return connection ID (SDK_CONNECT_SUCCESS)
JS->>Proxy: Poll for messages (GET /sw-cgi/websocket/poll/{connID})
Proxy->>Proxy: Retrieve queued messages
Proxy->>JS: Return Uint8Array stream (SDK_DATA)
JS->>Proxy: Send data (POST /sw-cgi/websocket/send/{connID})
Proxy->>SDK: Forward data to connection
SDK->>Upstream: Send data
else HTTP Proxy
Proxy->>DNS: Lookup address for hostname
DNS->>Proxy: Return resolved address
Proxy->>SDK: Dial HTTP endpoint
SDK->>Upstream: Connect & send request
Upstream->>SDK: Response data
SDK->>Proxy: Return response
Proxy->>JS: Response body as Uint8Array
end
JS->>Proxy: Disconnect (DELETE /sw-cgi/websocket/{connID})
Proxy->>SDK: Close connection
SDK->>Upstream: Gracefully close
Proxy->>Proxy: Remove from WebSocketManager
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
coderabbitai
bot
requested review from
gosunuts,
rabbitson87,
snowmerak,
splkm97 and
yoonhyunwoo
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| }() | ||
|
|
||
| lease, err := client.LookupName(leaseName) | ||
| if err != nil { |
There was a problem hiding this comment.
Production SDK skips lease-name normalization
Medium Severity
handleSDKConnect calls client.LookupName(leaseName) directly in production, omitting the getLeaseID normalization used in the non-prod path. This can break lookups for URL-encoded, punycode, or mixed-case lease names and cause production-only connection failures.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if err == io.EOF { | ||
| // Current message exhausted, try to get next one | ||
| g.currentReader = nil | ||
| continue | ||
| } |
There was a problem hiding this comment.
🔴 WsStream.Read discards data when message reader returns (n > 0, io.EOF)
When currentReader.Read(p) returns both data (n > 0) and io.EOF simultaneously—which is explicitly permitted by the io.Reader contract—the new code discards those n bytes by executing continue, which loops back and overwrites p from position 0 with data from the next WebSocket message.
Root Cause and Impact
The old code at portal/utils/wsstream/wsstream.go correctly handled this case:
n, err = g.currentReader.Read(p)
if err == io.EOF {
g.currentReader = nil
err = nil
}
return n, err // returns (n, nil) — data preservedThe new code at lines 57–62:
n, err = g.currentReader.Read(p)
if err == io.EOF {
g.currentReader = nil
continue // DISCARDS n bytes already written to p!
}Many io.Reader implementations return the final chunk of data together with io.EOF in a single call (e.g., bytes.Reader, and gorilla/websocket's internal message reader when the entire message fits in the buffer). When this happens, the n bytes already copied into p are silently lost because continue causes the loop to call NextReader() and then Read(p) again, overwriting p from index 0.
Impact: Silent data loss on every WebSocket stream used throughout the relay system (tunnel connections, SDK connections, webclient). This can cause corrupted HTTP responses, broken handshakes, or incomplete data transfers—essentially any communication flowing through WsStream.
| if err == io.EOF { | |
| // Current message exhausted, try to get next one | |
| g.currentReader = nil | |
| continue | |
| } | |
| if err == io.EOF { | |
| // Current message exhausted, clear reader for next call | |
| g.currentReader = nil | |
| // If data was read, return it (without the EOF error) | |
| if n > 0 { | |
| return n, nil | |
| } | |
| // No data in this message, try to get next one | |
| continue | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@cmd/webclient/main_js_prod.go`:
- Around line 425-432: The code uses io.ReadAll on proxied HTML responses (see
utils.IsHTMLContentType, resp.Body and io.ReadAll), which can exhaust WASM
memory; fix by replacing io.ReadAll(resp.Body) with reading from
io.LimitReader(resp.Body, maxHTMLBodyBytes) where maxHTMLBodyBytes is a
reasonable constant (e.g. a few MB), then handle the case where the response is
truncated (log or return a 502/413) before calling
w.WriteHeader(resp.StatusCode) and w.Write; ensure resp.Body is still closed and
errors from reading the limited reader are handled and surfaced to the client.
- Around line 603-607: The empty defer/recover closures in
cmd/webclient/main_js_prod.go (including the one in handleSDKConnect and the
three other similar closures) silently swallow panics; change each defer func()
{ if r := recover(); r != nil {} }() to capture the recovered value and at
minimum log it (e.g., console.Error or the package logger) with contextual
information and a stack trace (use runtime/debug.Stack()) so errors are visible
in production; keep the recover but replace the empty body with a log statement
that includes the recovered value and stack trace, or rethrow/propagate after
logging if the caller must handle it.
- Around line 414-417: The error response leaks an internal resolved URL via
r.URL.String() after calling httpClient.Do(r); remove the internal URL from the
client-facing message and return a generic error (e.g., "Failed to proxy
request") or use the original public host (r.Host or r.Header.Get("Host")) if
you must include a hostname, and separately log the detailed error and
r.URL.String()/err server-side for debugging; update the http.Error call that
currently uses fmt.Sprintf("Failed to proxy request to %s", r.URL.String()) and
add a process/server log of err and r.URL.String() instead.
- Around line 237-240: The dialer.Dial call in cmd/webclient/main_js_prod.go can
return a non-nil resp with a Body even when err != nil; modify the error path
after dialer.Dial(u.String(), nil) to close resp.Body if resp != nil before
returning the error (mirror the pattern used in utils/ws.go's
NewWebSocketDialer). Ensure you still return the original error and do not
swallow it; reference the variables dialer.Dial, resp, conn, and err when making
the change.
- Around line 406-412: The code uses r.Clone(context.Background()) which drops
the incoming request's cancellation; instead preserve the client's context by
cloning with r.Context() (e.g., r = r.Clone(r.Context())) or simply avoid
cloning and modify r.URL directly after reading the request context; ensure the
proxied request uses the original context so cancellation from the browser
propagates when you call getLeaseID and then set r.URL.Host and r.URL.Scheme.
- Around line 654-657: The code currently assigns connID := lease.Identity.Id
which keys sdkConnections by lease ID and causes silent overwrites; replace that
assignment with a call to generateConnID() so each connection gets a unique
identifier before storing it in sdkConnections (the block around
sdkConnectionsMu.Lock/Unlock that sets sdkConnections[connID] = conn). Ensure
connID is generated once per incoming SDK_CONNECT and used consistently for
registration and subsequent bookkeeping so existing connections are not
overwritten.
In `@portal/core/proto/rdsec/rdsec_test.go`:
- Around line 584-626: The test closures in testCases capture the outer
testing.T causing misattributed failures; change the testCases entry type from
"test func()" to "test func(t *testing.T)" and update each closure (for
ClientInitPayload, SignedPayload, ServerInitPayload) to accept a *testing.T
parameter and use that t for calls like t.Errorf/t.Error; finally change the
t.Run invocation to call tc.test(t) (i.e., t.Run(tc.name, func(t *testing.T) {
tc.test(t) })) so subtest failures are reported on the correct *testing.T.
- Around line 678-742: Replace the benchmark loops in
BenchmarkIdentity_MarshalVT, BenchmarkIdentity_UnmarshalVT,
BenchmarkClientInitPayload_MarshalVT, and BenchmarkClientInitPayload_UnmarshalVT
to use the recommended b.Loop() pattern instead of "for range b.N"; keep the
setup and b.ResetTimer() as-is, then change each loop body to "for b.Loop() {
... }" so the calls to msg.MarshalVT() and got.UnmarshalVT(data) execute inside
b.Loop().
- Around line 894-925: The test TestServerInitPayload_Reset currently asserts
msg.Version != ProtocolVersion_PROTOCOL_VERSION_1 which is misleading because
ProtocolVersion_PROTOCOL_VERSION_1 is the zero value; change the assertion to
explicitly check msg.Version == 0 (or != 0 if you intend a non-zero default) and
update the error message accordingly so it clearly asserts that Reset zeroes the
Version (referencing msg.Version and ProtocolVersion_PROTOCOL_VERSION_1 in the
TestServerInitPayload_Reset function).
In `@portal/core/proto/rdverb/rdverb_test.go`:
- Around line 974-1050: Replace the old benchmark loop pattern `for range b.N {
... }` with the new `for b.Loop() { ... }` in each benchmark function
(BenchmarkPacket_MarshalVT, BenchmarkPacket_UnmarshalVT,
BenchmarkRelayInfo_MarshalVT, BenchmarkLease_MarshalVT); keep the existing setup
(message creation, b.ResetTimer(), MarshalVT/UnmarshalVT calls) but change the
loop header so the body executes under `for b.Loop() { ... }` to comply with the
benchmarking guideline.
In `@portal/utils/wsstream/wsstream_test.go`:
- Around line 422-437: BenchmarkWsStream_Read is measuring the EOF error path
because newMockConn has only one message and readIndex is only reset every 1000
iterations; change the benchmark to use the b.Loop() pattern and ensure each
iteration reads valid data (e.g., reset mock.readIndex at the start of each loop
or create a fresh mock via newMockConn per loop) so WsStream.Read measures
successful reads rather than repeated io.EOF errors; update
BenchmarkWsStream_Read to use b.Loop() and either reset mock.readIndex or
recreate mock/stream inside the loop.
In `@portal/utils/wsstream/wsstream.go`:
- Around line 57-62: The Read wrapper currently discards bytes when
g.currentReader.Read(p) returns n>0 with err==io.EOF; change the logic in the
Read method handling of g.currentReader so that when err==io.EOF and n>0 you set
g.currentReader = nil and return n, nil (so the caller receives the final
bytes), and only when n==0 return 0, io.EOF; keep references to g.currentReader
and the Read loop logic to locate where to apply this change.
In `@utils/utils_test.go`:
- Around line 490-547: TestUpgradeWebSocket currently uses httptest.NewRecorder
which does not implement http.Hijacker so UpgradeWebSocket can never succeed;
either change the test to assert error paths only (rename the test to reflect it
validates error handling and remove the misleading success branch around conn)
or replace the recorder with a real HTTP server using httptest.NewServer that
performs an actual HTTP request to a handler which calls UpgradeWebSocket so the
ResponseWriter supports Hijacker and a real websocket upgrade/handshake can be
exercised; update assertions in TestUpgradeWebSocket accordingly and keep
references to the UpgradeWebSocket call and the test name to locate the change.
🧹 Nitpick comments (13)
portal/core/proto/rdverb/rdverb_test.go (1)
882-911:TestEmptyResponseMessagesduplicatesTestResponseCode_AllValues.Both test the exact same thing: round-trip marshal/unmarshal of
LeaseUpdateResponsefor everyResponseCodevalue. One of them should be removed.portal/core/proto/rdsec/rdsec_test.go (1)
478-512: Concurrent test uses goroutines that callt.Errorfvia closure over outert.This works because the
donechannel ensures all goroutines finish before the test returns, andt.Errorf(unliket.FailNow) is safe to call from non-test goroutines. That said, consider usingerrgroupor at minimum passingtexplicitly for clarity. Not blocking.justfile (3)
1-3:fmttarget is missinggoimports.The coding guidelines require
gofmt + goimportsfor formatting. This target only runsgofmt -w .. Addgoimports -w .(or use the golangci-lintgoimportsfixer) to stay compliant.Also,
> /dev/null || trueswallows all golangci-lint output and errors silently — consider at least redirecting to stderr or logging a note so developers aren't confused when fixes are applied but diagnostics vanish.Proposed fix
fmt: - golangci-lint run --fast-only --allow-parallel-runners --fix > /dev/null || true - gofmt -w . + golangci-lint run --fast-only --allow-parallel-runners --fix 2>/dev/null || true + goimports -w . + gofmt -w .As per coding guidelines: "Run formatting before commits using
make fmt(gofmt + goimports)"
5-6:linttarget disableserrcheck— intentional?The coding guidelines state "Never ignore errors unless explicitly excluded by errcheck." Disabling the errcheck linter entirely (
-D errcheck) means no error-ignoring violations will be caught duringjust lint. If this is a temporary measure, add a comment explaining why.
11-12:tidytarget is missinggo mod verify.The project guidelines specify
go mod tidy+go mod verifyfor module management.Proposed fix
tidy: go mod tidy + go mod verifyAs per coding guidelines (learning): "Run
make tidyfor Go module management (go mod tidy+go mod verify)"portal/utils/ratelimit/bucket_test.go (1)
304-309:Copyreturn error is silently dropped in concurrent test.Not a big deal in a test, but if
Copyfails for a reason other than rate limiting, this test won't catch it.Optional: capture and check errors
+ errs := make([]error, numGoroutines) + var mu sync.Mutex + i := 0 for range numGoroutines { go func() { defer wg.Done() src := bytes.NewReader(data) var dst bytes.Buffer - Copy(&dst, src, b) + _, err := Copy(&dst, src, b) + mu.Lock() + errs[i] = err + i++ + mu.Unlock() }() } wg.Wait() + for _, err := range errs { + if err != nil { + t.Errorf("unexpected Copy error: %v", err) + } + }.pre-commit-config.yaml (2)
47-55: Pre-commit hook only runsgofmt, missinggoimports.The
make fmttarget runs bothgofmtandgoimports, but this hook only runsgofmt -w .. Import ordering won't be enforced at commit time.Proposed fix
- repo: local hooks: - id: gofmt name: gofmt description: Formats Go source code. - entry: gofmt -w . + entry: bash -c 'gofmt -w . && goimports -w .' types: [go] language: system pass_filenames: falseAlternatively, just call
make fmtas the entry.As per coding guidelines: "Run formatting before commits using
make fmt(gofmt + goimports)".
24-39: Commented-out hooks are dead weight.These 16 lines of commented-out configuration add no value. Either remove them or add a note explaining why they're preserved for reference. Leaving them in just adds noise.
utils/utils_test.go (2)
477-488: Uset.Context()instead ofcontext.Background().Go 1.24+ provides
t.Context()which is automatically cancelled when the test ends.func TestNewWebSocketDialer(t *testing.T) { - ctx := context.Background() + ctx := t.Context() dialer := NewWebSocketDialer()This would also let you drop the
"context"import if unused elsewhere.As per coding guidelines: "Use
t.Context()in tests where applicable".
323-339: Overly complex assertion logic obscures test intent.This 6-condition boolean expression is hard to reason about. In practice, only the
"localhost:8080"case hits the "best-effort" path, and it still goes through theelsebranch because it contains:. Consider splitting hostname-resolution-dependent cases into a separate subtest guarded by a skip condition, rather than embedding this logic in every iteration.portal/utils/wsstream/wsstream.go (1)
46-52: Redundant nil check on innerif.Line 49's
if err != nil &&is always true here since the outerif err != nilon line 47 already guards this block.- if err != nil && strings.HasPrefix(err.Error(), "websocket: close ") { + if strings.HasPrefix(err.Error(), "websocket: close ") {portal/utils/wsstream/wsstream_test.go (1)
439-450: Usefor b.Loop() {}in Write benchmark.- for range b.N { + for b.Loop() { stream.Write(data) }Note: this benchmark also accumulates unbounded data in
mock.writeData. For largeb.N, memory use will balloon. Consider resetting or using a mock that discards writes.As per coding guidelines: "Use
for b.Loop() {}pattern in benchmarks".cmd/webclient/main_js_prod.go (1)
370-376:WriteMessageerror inClose()is silently ignored.If the connection is already dead, the close-message write will fail. That's fine for the happy path, but logging or at least acknowledging the error would help with debugging. Minor since
closeOnceprotects idempotency andconn.Close()follows regardless.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumportal/core/proto/rdsec/rdsec.pb.gois excluded by!**/*.pb.goportal/core/proto/rdsec/rdsec_vtproto.pb.gois excluded by!**/*.pb.goportal/core/proto/rdverb/rdverb.pb.gois excluded by!**/*.pb.goportal/core/proto/rdverb/rdverb_vtproto.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
.pre-commit-config.yamlMakefilecmd/portal-tunnel/main.gocmd/relay-server/manager/bps_manager.gocmd/webclient/inject.gocmd/webclient/main_js.gocmd/webclient/main_js_prod.gogo.modjustfileportal/core/cryptoops/handshaker.goportal/core/proto/rdsec/rdsec_test.goportal/core/proto/rdverb/rdverb_test.goportal/utils/ratelimit/bucket.goportal/utils/ratelimit/bucket_test.goportal/utils/wsstream/wsstream.goportal/utils/wsstream/wsstream_test.gosdk/sdk.goutils/utils_test.goutils/ws.go
🧰 Additional context used
📓 Path-based instructions (3)
go.mod
📄 CodeRabbit inference engine (AGENTS.md)
Pin Go toolchain version to match
go.mod(currently 1.25.3)
Files:
go.mod
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Run formatting before commits usingmake fmt(gofmt + goimports)
Import order must follow: stdlib -> external -> internal (blank-line separated)
Use lowercase single-word package names
Name interfaces as behavior verbs in Go
UseErrprefix for sentinel errors andErrorsuffix for error types in Go
Use context as first parameter for public I/O functions:func Do(ctx context.Context, ...)
Wrap errors with%wand include call-site context
Use sentinel errors per package and useerrors.Is/errors.Asfor error checking
Useerrors.Joinfor multi-error scenarios in Go
Never ignore errors unless explicitly excluded by errcheck
Use iterator signaturesfunc(yield func() bool),func(yield func(V) bool), orfunc(yield func(K, V) bool)for Go 1.23+ iterators
Always check yield return in iterators and prefer stdlib helpers likeslices.Collectandmaps.Keys
Prefererrgroup.Groupfor parallel work and useSetLimitfor bounds
No goroutines without clear exit; creator owns lifecycle
Use directional channels in function signatures; only sender closes channels
Avoidtime.Afterin loops; usecontext.WithTimeoutortime.Tickerinstead
Avoidmath/randfor security-sensitive operations in Go
Avoidreflecton hot paths; prefer generics or type switches
Usesync.Poolonly on hot paths for performance optimization
Files:
sdk/sdk.goportal/utils/ratelimit/bucket.goutils/utils_test.goportal/utils/wsstream/wsstream_test.goportal/core/proto/rdverb/rdverb_test.gocmd/portal-tunnel/main.gocmd/relay-server/manager/bps_manager.goutils/ws.goportal/core/proto/rdsec/rdsec_test.goportal/utils/wsstream/wsstream.gocmd/webclient/inject.goportal/utils/ratelimit/bucket_test.gocmd/webclient/main_js.goportal/core/cryptoops/handshaker.gocmd/webclient/main_js_prod.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Use race detector in normal test runs withgo test -race
Uset.Context()in tests where applicable
Usefor b.Loop() {}pattern in benchmarks
Files:
utils/utils_test.goportal/utils/wsstream/wsstream_test.goportal/core/proto/rdverb/rdverb_test.goportal/core/proto/rdsec/rdsec_test.goportal/utils/ratelimit/bucket_test.go
🧠 Learnings (20)
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to go.mod : Pin Go toolchain version to match `go.mod` (currently 1.25.3)
Applied to files:
go.mod
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*.go : Import order must follow: stdlib -> external -> internal (blank-line separated)
Applied to files:
go.mod
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*.go : Avoid `math/rand` for security-sensitive operations in Go
Applied to files:
go.modcmd/webclient/inject.gocmd/webclient/main_js.go
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*.go : Prefer `errgroup.Group` for parallel work and use `SetLimit` for bounds
Applied to files:
go.modportal/utils/ratelimit/bucket_test.go
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*.go : Use `sync.Pool` only on hot paths for performance optimization
Applied to files:
portal/utils/ratelimit/bucket.gocmd/portal-tunnel/main.gocmd/relay-server/manager/bps_manager.goportal/core/cryptoops/handshaker.go
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*_test.go : Use `t.Context()` in tests where applicable
Applied to files:
utils/utils_test.go
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Run `make build` to build all artifacts, or use specific targets: `make build-server`, `make build-frontend`, `make build-wasm`, `make build-tunnel`, `make build-protoc`
Applied to files:
Makefile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: For frontend development in `cmd/relay-server/frontend/`, use `npm run dev` for development, `npm run build` for production builds, and `npm run lint` for linting
Applied to files:
Makefile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*.go : Run formatting before commits using `make fmt` (gofmt + goimports)
Applied to files:
.pre-commit-config.yamljustfile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to .golangci.yml : Use linter tiers configuration in `.golangci.yml` for correctness, quality, concurrency safety, and performance/modernization checks
Applied to files:
.pre-commit-config.yamljustfile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Run `make lint` for golangci-lint analysis
Applied to files:
.pre-commit-config.yamljustfile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*.go : Avoid `reflect` on hot paths; prefer generics or type switches
Applied to files:
cmd/webclient/inject.gocmd/webclient/main_js.go
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*_test.go : Use `for b.Loop() {}` pattern in benchmarks
Applied to files:
cmd/webclient/inject.gocmd/webclient/main_js.go
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Use `go vet`, `golangci-lint`, `go test -race`, and `govulncheck` for static analysis
Applied to files:
justfile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Run `make vet` for Go vet analysis
Applied to files:
justfile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Run `make tidy` for Go module management (`go mod tidy` + `go mod verify`)
Applied to files:
justfile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Run `make test` for Go tests with `-v -race` flags
Applied to files:
justfile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*.go : Never ignore errors unless explicitly excluded by errcheck
Applied to files:
justfile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*_test.go : Use race detector in normal test runs with `go test -race`
Applied to files:
justfile
📚 Learning: 2026-01-27T12:45:06.366Z
Learnt from: CR
Repo: gosuda/portal PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-27T12:45:06.366Z
Learning: Applies to **/*.go : No goroutines without clear exit; creator owns lifecycle
Applied to files:
cmd/webclient/main_js_prod.go
🧬 Code graph analysis (8)
utils/utils_test.go (3)
utils/http.go (3)
IsHTMLContentType(12-21)SetCORSHeaders(50-54)IsLocalhost(56-81)utils/url.go (5)
IsHexString(93-100)StripWildCard(138-142)DefaultAppPattern(170-184)DefaultBootstrapFrom(193-206)StripPort(144-162)utils/ws.go (3)
NewWebSocketDialer(15-27)UpgradeWebSocket(35-37)UpgradeToWSStream(40-46)
portal/utils/wsstream/wsstream_test.go (1)
portal/utils/wsstream/wsstream.go (2)
WsStream(20-25)New(28-32)
utils/ws.go (1)
portal/utils/wsstream/wsstream.go (1)
New(28-32)
portal/core/proto/rdsec/rdsec_test.go (1)
portal/core/proto/rdsec/rdsec.pb.go (6)
Identity(40-44)ClientInitPayload(66-74)ProtocolVersion_PROTOCOL_VERSION_1(13-13)SignedPayload(124-128)ServerInitPayload(150-158)ProtocolVersion(10-10)
portal/utils/wsstream/wsstream.go (1)
cmd/webclient/wsjs/ws_js.go (1)
Conn(19-26)
portal/utils/ratelimit/bucket_test.go (1)
portal/utils/ratelimit/bucket.go (3)
NewBucket(21-37)Bucket(12-17)Copy(70-101)
portal/core/cryptoops/handshaker.go (1)
portal/core/proto/rdsec/rdsec.pb.go (2)
ServerInitPayload(150-158)ClientInitPayload(66-74)
cmd/webclient/main_js_prod.go (3)
cmd/webclient/main_js.go (7)
WebSocketManager(197-199)StreamMessage(235-241)NewWebSocketManager(243-245)WSConnection(201-211)ConnectRequest(218-221)ConnectResponse(223-226)SendRequest(228-233)utils/http.go (1)
IsHTMLContentType(12-21)cmd/webclient/sdk_js.go (1)
WebSocketDialerJS(11-22)
🔍 Remote MCP Github Grep
Summary of Relevant Context for PR Review
Based on searching for concrete patterns and best practices used in production systems, here are the key findings that validate the PR's technical approach:
1. sync.Pool Pointer Pattern (Buffer Pool Optimizations)
The PR's change to store *[]byte pointers in sync.Pool instead of plain []byte values is a well-established optimization pattern used across production systems like containerd, ethereum-optimism, and gvisor. The standard retrieval pattern—bufPool.Get().(*[]byte), dereference to []byte, then defer bufPool.Put(p)—is confirmed in multiple high-profile projects. This reduces interface boxing allocations and is considered a standard best practice for performance-critical buffer management.
2. VTProto Migration (Protobuf Serialization)
The migration from reflection-based protobuf to vtproto (MarshalVT/UnmarshalVT) is designed for reflection-free, TinyGo-compatible implementations that are more efficient at runtime and result in smaller code binaries, with better support for TinyGo's limited reflection capabilities. [::web_search::] This approach eliminates reflection usage and generates optimized code for serialization/deserialization for each specific message, a pattern used by major systems like Vitess and etcd.
3. go:build !prod Build Constraint Pattern
The pattern of using //go:build !prod to exclude development-only code (injection, debugging) from production builds is a standard approach used in enterprise projects (confirmed in Mattermost, Keybase). This ensures production binaries remain lean and deterministic.
4. HTTP Response Body Resource Management
The addition of resp.Body.Close() in error paths follows Go's standard resource management pattern confirmed across numerous production projects (Vault, Harbor, GitHub repositories), preventing connection leaks.
5. WASM Optimization Strategy (Dual-Pass)
The PR's dual-pass wasm-opt approach (O4 for runtime performance, then Oz for aggressive size reduction) aligns with wasm-opt's optimization levels: -O4 applies aggressive transformations to flatten the IR and optimize complex code, while -Oz focuses on super-aggressive code size reduction. [::web_search::] This production-level optimization strategy, though requiring longer compilation time due to depth of transformations, results in highly efficient code that executes faster and is usually smaller in size.
6. New WebSocket Interface Pattern
The refactoring of WsStream to use a mockable webSocketConn interface follows Go's dependency injection pattern, enabling easier testing while preserving production use of gorilla/websocket. This is validated as a standard practice for improving testability without performance overhead.
🔇 Additional comments (25)
sdk/sdk.go (2)
107-112: LGTM — clean pre-allocation under the lock.Pre-sizing the slice to
len(g.relays)avoids needless grow-and-copy cycles during the append loop. Consistent with the same pattern inClose(). No issues.
629-634: LGTM — same pre-allocation pattern applied consistently.Matches the
Dialchange. Lock scope is tight, slice is iterated outside the critical section. No issues.portal/core/cryptoops/handshaker.go (2)
387-387: VT migration inServerHandshakeis equally clean.Same mechanical swap on the server side —
UnmarshalVTfor incomingSignedPayload/ClientInitPayload,MarshalVTfor outgoingServerInitPayload/SignedPayload. Consistent with the client path. No concerns.Also applies to: 393-393, 430-430, 454-454
318-318: VT migration inClientHandshakeis correct.All MarshalVT/UnmarshalVT call sites are verified. The three proto types (
ClientInitPayload,SignedPayload,ServerInitPayload) all have the required methods generated inrdsec_vtproto.pb.go. The 1:1 swap from reflection-basedproto.Marshal/proto.Unmarshalis complete across all eight changed lines, with error handling preserved.portal/core/proto/rdverb/rdverb_test.go (3)
3-8: Import order and structure look correct.stdlib (
bytes,testing) followed by external (rdsec) with blank-line separation. No issues here.
504-576: Solid deep-clone independence tests.Good coverage verifying that modifying cloned
Packet,RelayInfo, andLeasefields (including nested identity and slice elements) doesn't affect the original. This is exactly the kind of test that catches shallow-copy bugs.
762-833: Thorough nil-handling coverage across all message types.Every public message type is checked for nil
CloneVT, nilMarshalVT, and nilSizeVT. Nothing missing.portal/core/proto/rdsec/rdsec_test.go (2)
1-6: Clean imports, correct package name.stdlib-only imports, properly organized.
8-64: Good round-trip coverage forIdentity.Covers empty, full, partial-field cases. The
wantErrpattern is consistent and the equality check viaEqualVTis appropriate.cmd/webclient/main_js.go (1)
1-2: Build tag looks correct.The
//go:build !prodconstraint properly excludes this file from production builds, working in tandem withmain_js_prod.go. No issues here.portal/utils/ratelimit/bucket.go (1)
62-75:sync.Poolpointer pattern is correctly applied.The
*[]bytepool pattern avoids interface boxing allocations — standard optimization. Retrieval, dereference, and deferred return are all correct.go.mod (1)
12-15: Dependency bumps look routine.Minor version bumps for
x/crypto,x/net,protobuf, andbroccoli. No concerns.cmd/relay-server/manager/bps_manager.go (1)
334-348: Same*[]bytepool optimization — correctly applied, consistent withratelimit/bucket.go.Pattern mirrors the identical change in
portal/utils/ratelimit/bucket.go. No issues.Side note: this file duplicates
Bucket,NewBucket,Take,Copy, andbufPoolfromportal/utils/ratelimit/bucket.go(with a different Bucket algorithm). If these were consolidated, you'd only need to apply this kind of change once. Not blocking — just a maintenance observation.utils/ws.go (2)
17-25: Resource leak fix — good catch closingresp.Bodyon error.The gorilla/websocket
Dialcan return a non-nil response even on error. Closingresp.Bodyin the error path prevents connection leaks. The factory switch towsstream.New(wsConn)is clean.
45-45: Consistent factory usage.
wsstream.New(wsConn)aligns with the refactored constructor inwsstream.go.cmd/portal-tunnel/main.go (2)
22-28: Consistent*[]bytepool pattern across the codebase.Same correct optimization applied here. Local
bufper goroutine — no aliasing risk.
206-218: Buffer retrieval in goroutines is safe.Each goroutine gets its own local
buffrom the pool and defers the return. Theio.CopyBuffercall uses the dereferenced slice correctly. No concerns.portal/utils/ratelimit/bucket_test.go (3)
40-60: Good coverage of invalid rate edge cases.Table-driven tests for zero and negative rates — clean.
399-416:shortWriterreturns both bytes written and an error simultaneously.This works for this test because
Copychecksew != nilbeforenr != nw, so the write error path is hit. But note: if theCopyimplementation ever reorders those checks, the assertion onio.ErrShortWrite(line 279) could break. The test is correct as-is given the currentCopyimplementation — just be aware of the coupling.
418-423:errWriter.Writereturnslen(p)alongside a non-nil error.This is a valid (if unusual)
io.Writerbehavior. TheCopycode path returns the write error after counting the bytes — which is exactly whatTestCopyWriteErrorasserts (lines 329-337). Correct.cmd/webclient/inject.go (1)
1-2: Build constraint looks correct.Clean separation of dev-only code via
//go:build !prod. Pairs properly with the prod-taggedmain_js_prod.go.portal/utils/wsstream/wsstream.go (1)
11-17: Interface design is clean and minimal.Good abstraction — three methods, unexported, mockable. Covers exactly the surface area
WsStreamneeds.portal/utils/wsstream/wsstream_test.go (1)
70-232: Solid test coverage for Read.Good variety: single message, partial reads, multi-message, empty buffer, EOF, errors, close errors, and concurrency. Well structured.
cmd/webclient/main_js_prod.go (1)
811-866: Main function setup and blocking pattern look correct.Bootstrap server discovery, SDK client init with defer close, proxy wiring, JS interop hooks, and the TinyGo guard + blocking channel are all properly structured.
Makefile (1)
62-67: The-gcflags="-l=4"syntax is valid and correct.The
-l=4flag enables aggressive mid-stack inlining in Go's compiler—it's the proper numeric form to relax inlining costs for non-leaf functions. Your Makefile is correct here, not a concern.The
-trimpathinside-gcflagsis technically redundant with the top-level-trimpathflag (which already sets it for both gcflags and asmflags), but it causes no harm.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| conn, resp, err := dialer.Dial(u.String(), nil) | ||
| if err != nil { | ||
| return nil, "", err | ||
| } |
There was a problem hiding this comment.
Resource leak: resp.Body not closed on dial error.
When dialer.Dial fails, the HTTP response may still have a non-nil body that needs closing — exactly the pattern already fixed in utils/ws.go's NewWebSocketDialer.
Proposed fix
conn, resp, err := dialer.Dial(u.String(), nil)
if err != nil {
+ if resp != nil {
+ resp.Body.Close()
+ }
return nil, "", err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| conn, resp, err := dialer.Dial(u.String(), nil) | |
| if err != nil { | |
| return nil, "", err | |
| } | |
| conn, resp, err := dialer.Dial(u.String(), nil) | |
| if err != nil { | |
| if resp != nil { | |
| resp.Body.Close() | |
| } | |
| return nil, "", err | |
| } |
🤖 Prompt for AI Agents
In `@cmd/webclient/main_js_prod.go` around lines 237 - 240, The dialer.Dial call
in cmd/webclient/main_js_prod.go can return a non-nil resp with a Body even when
err != nil; modify the error path after dialer.Dial(u.String(), nil) to close
resp.Body if resp != nil before returning the error (mirror the pattern used in
utils/ws.go's NewWebSocketDialer). Ensure you still return the original error
and do not swallow it; reference the variables dialer.Dial, resp, conn, and err
when making the change.
| } | ||
|
|
||
| r = r.Clone(context.Background()) | ||
|
|
||
| decodedHost := getLeaseID(r.URL.Hostname()) | ||
| r.URL.Host = decodedHost | ||
| r.URL.Scheme = "http" |
There was a problem hiding this comment.
context.Background() drops the original request's cancellation signal.
r.Clone(context.Background()) severs the connection to the client's context. If the browser disconnects, the proxied request will continue until httpClient.Timeout (30s) expires instead of cancelling promptly. Use the incoming request's context or derive from it.
- r = r.Clone(context.Background())
+ r = r.Clone(r.Context())Or simply don't clone the context at all if you only need to modify URL fields.
🤖 Prompt for AI Agents
In `@cmd/webclient/main_js_prod.go` around lines 406 - 412, The code uses
r.Clone(context.Background()) which drops the incoming request's cancellation;
instead preserve the client's context by cloning with r.Context() (e.g., r =
r.Clone(r.Context())) or simply avoid cloning and modify r.URL directly after
reading the request context; ensure the proxied request uses the original
context so cancellation from the browser propagates when you call getLeaseID and
then set r.URL.Host and r.URL.Scheme.
| resp, err := httpClient.Do(r) | ||
| if err != nil { | ||
| http.Error(w, fmt.Sprintf("Failed to proxy request to %s", r.URL.String()), http.StatusBadGateway) | ||
| return |
There was a problem hiding this comment.
Internal resolved URL leaked in error response.
r.URL.String() at this point contains the resolved lease ID (from getLeaseID), not the original hostname. This exposes internal routing details to the client.
🤖 Prompt for AI Agents
In `@cmd/webclient/main_js_prod.go` around lines 414 - 417, The error response
leaks an internal resolved URL via r.URL.String() after calling
httpClient.Do(r); remove the internal URL from the client-facing message and
return a generic error (e.g., "Failed to proxy request") or use the original
public host (r.Host or r.Header.Get("Host")) if you must include a hostname, and
separately log the detailed error and r.URL.String()/err server-side for
debugging; update the http.Error call that currently uses fmt.Sprintf("Failed to
proxy request to %s", r.URL.String()) and add a process/server log of err and
r.URL.String() instead.
| if utils.IsHTMLContentType(resp.Header.Get("Content-Type")) { | ||
| w.WriteHeader(resp.StatusCode) | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return | ||
| } | ||
| w.Write(body) | ||
| return |
There was a problem hiding this comment.
Unbounded io.ReadAll on proxied HTML responses.
A malicious or misbehaving upstream could return an arbitrarily large HTML body, exhausting WASM memory. Consider using io.LimitReader with a reasonable cap.
🤖 Prompt for AI Agents
In `@cmd/webclient/main_js_prod.go` around lines 425 - 432, The code uses
io.ReadAll on proxied HTML responses (see utils.IsHTMLContentType, resp.Body and
io.ReadAll), which can exhaust WASM memory; fix by replacing
io.ReadAll(resp.Body) with reading from io.LimitReader(resp.Body,
maxHTMLBodyBytes) where maxHTMLBodyBytes is a reasonable constant (e.g. a few
MB), then handle the case where the response is truncated (log or return a
502/413) before calling w.WriteHeader(resp.StatusCode) and w.Write; ensure
resp.Body is still closed and errors from reading the limited reader are handled
and surfaced to the client.
| func handleSDKConnect(data js.Value) { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| } | ||
| }() |
There was a problem hiding this comment.
Empty recover() blocks silently swallow panics.
Four locations have defer func() { if r := recover(); r != nil {} }() that catch and discard all panics without logging, metrics, or any signal. This makes debugging production failures extremely difficult. At minimum, log the recovered value.
Also applies to: 627-630, 722-726, 777-781
🤖 Prompt for AI Agents
In `@cmd/webclient/main_js_prod.go` around lines 603 - 607, The empty
defer/recover closures in cmd/webclient/main_js_prod.go (including the one in
handleSDKConnect and the three other similar closures) silently swallow panics;
change each defer func() { if r := recover(); r != nil {} }() to capture the
recovered value and at minimum log it (e.g., console.Error or the package
logger) with contextual information and a stack trace (use
runtime/debug.Stack()) so errors are visible in production; keep the recover but
replace the empty body with a log statement that includes the recovered value
and stack trace, or rethrow/propagate after logging if the caller must handle
it.
| // TestServerInitPayload_Reset tests Reset method | ||
| func TestServerInitPayload_Reset(t *testing.T) { | ||
| msg := &ServerInitPayload{ | ||
| Version: ProtocolVersion_PROTOCOL_VERSION_1, | ||
| Nonce: []byte{0x01}, | ||
| Timestamp: 123, | ||
| Identity: &Identity{Id: "test"}, | ||
| Alpn: "h2", | ||
| SessionPublicKey: []byte{0xAA}, | ||
| } | ||
|
|
||
| msg.Reset() | ||
|
|
||
| if msg.Version != ProtocolVersion_PROTOCOL_VERSION_1 { | ||
| t.Error("Reset() changed Version from default") | ||
| } | ||
| if msg.Nonce != nil { | ||
| t.Error("Reset() did not clear Nonce") | ||
| } | ||
| if msg.Timestamp != 0 { | ||
| t.Error("Reset() did not clear Timestamp") | ||
| } | ||
| if msg.Identity != nil { | ||
| t.Error("Reset() did not clear Identity") | ||
| } | ||
| if msg.Alpn != "" { | ||
| t.Error("Reset() did not clear Alpn") | ||
| } | ||
| if msg.SessionPublicKey != nil { | ||
| t.Error("Reset() did not clear SessionPublicKey") | ||
| } | ||
| } |
There was a problem hiding this comment.
TestServerInitPayload_Reset assertion for Version is subtly misleading.
Line 907 asserts msg.Version != ProtocolVersion_PROTOCOL_VERSION_1 after Reset(). Since PROTOCOL_VERSION_1 is 0 (the zero value), this doesn't actually verify that Reset() preserved the version — it verifies it was zeroed out, which coincidentally equals PROTOCOL_VERSION_1. The error message "Reset() changed Version from default" suggests the opposite intent. If the enum ever gains a non-zero default, this test silently passes incorrectly.
Consider explicitly checking against 0:
- if msg.Version != ProtocolVersion_PROTOCOL_VERSION_1 {
- t.Error("Reset() changed Version from default")
+ if msg.Version != 0 {
+ t.Error("Reset() did not clear Version")
}🤖 Prompt for AI Agents
In `@portal/core/proto/rdsec/rdsec_test.go` around lines 894 - 925, The test
TestServerInitPayload_Reset currently asserts msg.Version !=
ProtocolVersion_PROTOCOL_VERSION_1 which is misleading because
ProtocolVersion_PROTOCOL_VERSION_1 is the zero value; change the assertion to
explicitly check msg.Version == 0 (or != 0 if you intend a non-zero default) and
update the error message accordingly so it clearly asserts that Reset zeroes the
Version (referencing msg.Version and ProtocolVersion_PROTOCOL_VERSION_1 in the
TestServerInitPayload_Reset function).
| // BenchmarkPacket_MarshalVT benchmarks packet marshaling | ||
| func BenchmarkPacket_MarshalVT(b *testing.B) { | ||
| msg := &Packet{ | ||
| Type: PacketType_PACKET_TYPE_CONNECTION_REQUEST, | ||
| Payload: bytes.Repeat([]byte{0x01}, 1024), | ||
| } | ||
|
|
||
| b.ResetTimer() | ||
| for range b.N { | ||
| _, _ = msg.MarshalVT() | ||
| } | ||
| } | ||
|
|
||
| // BenchmarkPacket_UnmarshalVT benchmarks packet unmarshaling | ||
| func BenchmarkPacket_UnmarshalVT(b *testing.B) { | ||
| msg := &Packet{ | ||
| Type: PacketType_PACKET_TYPE_CONNECTION_REQUEST, | ||
| Payload: bytes.Repeat([]byte{0x01}, 1024), | ||
| } | ||
|
|
||
| data, _ := msg.MarshalVT() | ||
|
|
||
| b.ResetTimer() | ||
| for range b.N { | ||
| got := &Packet{} | ||
| _ = got.UnmarshalVT(data) | ||
| } | ||
| } | ||
|
|
||
| // BenchmarkRelayInfo_MarshalVT benchmarks complex relay info marshaling | ||
| func BenchmarkRelayInfo_MarshalVT(b *testing.B) { | ||
| msg := &RelayInfo{ | ||
| Identity: &rdsec.Identity{ | ||
| Id: "benchmark-relay", | ||
| PublicKey: bytes.Repeat([]byte{0xAA}, 32), | ||
| }, | ||
| Address: []string{"addr1:8080", "addr2:8080", "addr3:8080"}, | ||
| Leases: []*Lease{ | ||
| { | ||
| Identity: &rdsec.Identity{Id: "l1", PublicKey: bytes.Repeat([]byte{0x01}, 32)}, | ||
| Expires: 1234567890, | ||
| Name: "lease1", | ||
| Alpn: []string{"h2", "grpc"}, | ||
| }, | ||
| { | ||
| Identity: &rdsec.Identity{Id: "l2", PublicKey: bytes.Repeat([]byte{0x02}, 32)}, | ||
| Expires: 9876543210, | ||
| Name: "lease2", | ||
| Alpn: []string{"h2"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| b.ResetTimer() | ||
| for range b.N { | ||
| _, _ = msg.MarshalVT() | ||
| } | ||
| } | ||
|
|
||
| // BenchmarkLease_MarshalVT benchmarks lease marshaling | ||
| func BenchmarkLease_MarshalVT(b *testing.B) { | ||
| msg := &Lease{ | ||
| Identity: &rdsec.Identity{ | ||
| Id: "benchmark-lease", | ||
| PublicKey: bytes.Repeat([]byte{0xBB}, 32), | ||
| }, | ||
| Expires: 1234567890, | ||
| Name: "benchmark-lease", | ||
| Alpn: []string{"h2", "grpc", "http/1.1"}, | ||
| Metadata: "benchmark metadata", | ||
| } | ||
|
|
||
| b.ResetTimer() | ||
| for range b.N { | ||
| _, _ = msg.MarshalVT() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Benchmarks must use for b.Loop() {} instead of for range b.N.
All four benchmarks (BenchmarkPacket_MarshalVT, BenchmarkPacket_UnmarshalVT, BenchmarkRelayInfo_MarshalVT, BenchmarkLease_MarshalVT) use the old for range b.N loop pattern. The coding guidelines require for b.Loop() {}.
Example fix for BenchmarkPacket_MarshalVT (apply same pattern to all four)
func BenchmarkPacket_MarshalVT(b *testing.B) {
msg := &Packet{
Type: PacketType_PACKET_TYPE_CONNECTION_REQUEST,
Payload: bytes.Repeat([]byte{0x01}, 1024),
}
b.ResetTimer()
- for range b.N {
+ for b.Loop() {
_, _ = msg.MarshalVT()
}
}As per coding guidelines, "Use for b.Loop() {} pattern in benchmarks".
🤖 Prompt for AI Agents
In `@portal/core/proto/rdverb/rdverb_test.go` around lines 974 - 1050, Replace the
old benchmark loop pattern `for range b.N { ... }` with the new `for b.Loop() {
... }` in each benchmark function (BenchmarkPacket_MarshalVT,
BenchmarkPacket_UnmarshalVT, BenchmarkRelayInfo_MarshalVT,
BenchmarkLease_MarshalVT); keep the existing setup (message creation,
b.ResetTimer(), MarshalVT/UnmarshalVT calls) but change the loop header so the
body executes under `for b.Loop() { ... }` to comply with the benchmarking
guideline.
| func BenchmarkWsStream_Read(b *testing.B) { | ||
| data := make([]byte, 1024) | ||
| mock := newMockConn(data) | ||
| stream := &WsStream{Conn: mock} | ||
|
|
||
| buf := make([]byte, 1024) | ||
|
|
||
| b.ResetTimer() | ||
| for i := range b.N { | ||
| stream.Read(buf) | ||
| // Reset for next iteration | ||
| if i%1000 == 999 { | ||
| mock.readIndex = 0 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Read benchmark is broken — measures the EOF error path ~999/1000 iterations.
The mock has one message, so after the first Read succeeds, all subsequent calls hit NextReader → io.EOF until readIndex resets every 1000th iteration. You're benchmarking error returns, not reads.
Also, use for b.Loop() {} per project guidelines instead of for i := range b.N.
Proposed fix
func BenchmarkWsStream_Read(b *testing.B) {
data := make([]byte, 1024)
- mock := newMockConn(data)
- stream := &WsStream{Conn: mock}
-
buf := make([]byte, 1024)
b.ResetTimer()
- for i := range b.N {
- stream.Read(buf)
- // Reset for next iteration
- if i%1000 == 999 {
- mock.readIndex = 0
- }
+ for b.Loop() {
+ mock := newMockConn(data)
+ stream := &WsStream{Conn: mock}
+ stream.Read(buf)
}
}As per coding guidelines: "Use for b.Loop() {} pattern in benchmarks".
🤖 Prompt for AI Agents
In `@portal/utils/wsstream/wsstream_test.go` around lines 422 - 437,
BenchmarkWsStream_Read is measuring the EOF error path because newMockConn has
only one message and readIndex is only reset every 1000 iterations; change the
benchmark to use the b.Loop() pattern and ensure each iteration reads valid data
(e.g., reset mock.readIndex at the start of each loop or create a fresh mock via
newMockConn per loop) so WsStream.Read measures successful reads rather than
repeated io.EOF errors; update BenchmarkWsStream_Read to use b.Loop() and either
reset mock.readIndex or recreate mock/stream inside the loop.
| n, err = g.currentReader.Read(p) | ||
| if err == io.EOF { | ||
| // Current message exhausted, try to get next one | ||
| g.currentReader = nil | ||
| continue | ||
| } |
There was a problem hiding this comment.
Data loss when Read returns n > 0 alongside io.EOF.
The io.Reader contract permits returning n > 0, io.EOF simultaneously (i.e., the last chunk of data with EOF). This code discards any bytes read when err == io.EOF because it jumps straight to continue without returning n.
This won't surface with bytes.Reader (which returns 0, io.EOF separately), but any reader that signals EOF alongside the final data chunk will silently lose bytes.
Proposed fix
n, err = g.currentReader.Read(p)
if err == io.EOF {
- // Current message exhausted, try to get next one
g.currentReader = nil
- continue
+ if n > 0 {
+ return n, nil
+ }
+ continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| n, err = g.currentReader.Read(p) | |
| if err == io.EOF { | |
| // Current message exhausted, try to get next one | |
| g.currentReader = nil | |
| continue | |
| } | |
| n, err = g.currentReader.Read(p) | |
| if err == io.EOF { | |
| g.currentReader = nil | |
| if n > 0 { | |
| return n, nil | |
| } | |
| continue | |
| } |
🤖 Prompt for AI Agents
In `@portal/utils/wsstream/wsstream.go` around lines 57 - 62, The Read wrapper
currently discards bytes when g.currentReader.Read(p) returns n>0 with
err==io.EOF; change the logic in the Read method handling of g.currentReader so
that when err==io.EOF and n>0 you set g.currentReader = nil and return n, nil
(so the caller receives the final bytes), and only when n==0 return 0, io.EOF;
keep references to g.currentReader and the Read loop logic to locate where to
apply this change.
| func TestUpgradeWebSocket(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| requestHeaders map[string]string | ||
| expectError bool | ||
| }{ | ||
| { | ||
| name: "valid websocket upgrade request", | ||
| requestHeaders: map[string]string{ | ||
| "Connection": "Upgrade", | ||
| "Upgrade": "websocket", | ||
| "Sec-WebSocket-Version": "13", | ||
| "Sec-WebSocket-Key": "dGhlIHNhbXBsZSBub25jZQ==", | ||
| }, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "missing upgrade header", | ||
| requestHeaders: map[string]string{ | ||
| "Connection": "Upgrade", | ||
| }, | ||
| expectError: true, | ||
| }, | ||
| { | ||
| name: "no headers", | ||
| requestHeaders: map[string]string{}, | ||
| expectError: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| req := httptest.NewRequest("GET", "/", nil) | ||
| for k, v := range tt.requestHeaders { | ||
| req.Header.Set(k, v) | ||
| } | ||
|
|
||
| w := httptest.NewRecorder() | ||
|
|
||
| conn, err := UpgradeWebSocket(w, req, nil) | ||
|
|
||
| if tt.expectError { | ||
| assert.Error(t, err) | ||
| assert.Nil(t, conn) | ||
| } else { | ||
| // If no error, we should get a connection | ||
| // Note: The response might have been written already | ||
| if err == nil { | ||
| assert.NotNil(t, conn) | ||
| conn.Close() | ||
| } else { | ||
| // Some error cases are acceptable in test environment | ||
| assert.NotNil(t, err) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
WebSocket upgrade tests will never succeed with httptest.NewRecorder.
httptest.NewRecorder doesn't implement http.Hijacker, so websocket.Upgrade will always fail. The "valid websocket upgrade request" case (line 497) is misleading — it can never reach the success path. The fallback logic on lines 537-543 silently swallows the inevitable error, making this a no-op assertion.
Either use httptest.NewServer with a real HTTP server to test actual upgrades, or rename the test to reflect it only validates error handling.
🤖 Prompt for AI Agents
In `@utils/utils_test.go` around lines 490 - 547, TestUpgradeWebSocket currently
uses httptest.NewRecorder which does not implement http.Hijacker so
UpgradeWebSocket can never succeed; either change the test to assert error paths
only (rename the test to reflect it validates error handling and remove the
misleading success branch around conn) or replace the recorder with a real HTTP
server using httptest.NewServer that performs an actual HTTP request to a
handler which calls UpgradeWebSocket so the ResponseWriter supports Hijacker and
a real websocket upgrade/handshake can be exercised; update assertions in
TestUpgradeWebSocket accordingly and keep references to the UpgradeWebSocket
call and the test name to locate the change.
2 participants
This pull request introduces several improvements and optimizations across the codebase. The most significant changes include major performance optimizations for buffer pooling, a refactored WebAssembly (WASM) production build process, a migration to reflection-free protobufs for TinyGo compatibility, and enhanced developer tooling for code quality and consistency.
Performance and Memory Optimizations:
cmd/portal-tunnel/main.goandcmd/relay-server/manager/bps_manager.goto use pointers to byte slices (*[]byte) instead of slices directly, reducing interface boxing allocations and improving performance under high concurrency. [1] [2] [3] [4]WebAssembly (WASM) Build Improvements:
build-wasmtarget in theMakefileto produce production-ready WASM builds by stripping debug symbols, enabling aggressive inlining, setting reproducible build IDs, and adding aprodbuild tag to disable debugging and HTML injection. Also improved WASM optimization by running a dual-pass withwasm-optfor both speed and size.//go:build !prod) tocmd/webclient/inject.goandcmd/webclient/main_js.goto exclude debugging and injection code from production builds. [1] [2]Protobuf and Protocol Changes:
portal/core/proto/rdsec/rdsec.pb.goto a reflection-free, TinyGo-compatible protobuf implementation by removing dependencies onprotoimplandprotoreflect, simplifying message types, and updating serialization/deserialization code inportal/core/cryptoops/handshaker.goto useMarshalVT/UnmarshalVTmethods. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Developer Tooling and Code Quality:
.pre-commit-config.yamlwith hooks for whitespace cleanup, line ending normalization, Go formatting, and configuration verification, as well as a newjustfilewith common formatting, linting, testing, and vetting commands to streamline development workflows. [1] [2]Dependency Updates:
go.mod, includinggolang.org/x/crypto,golang.org/x/net,google.golang.org/protobuf,gopkg.eu.org/broccoli, and indirect dependencies, to newer versions for improved security and compatibility.Note
High Risk
Touches security- and correctness-critical areas (handshake/protocol serialization) and introduces a new prod-specific WASM entrypoint, so regressions could break connectivity or client behavior in production.
Overview
Pre-TinyGo build + runtime optimizations across the Go/WASM client and core protocol code.
WASM builds now have a production mode (
-tags=prod) with a newcmd/webclient/main_js_prod.go(no logging, no HTML injection, includes a websocket polyfill proxy + SDK message handler) and an updatedMakefilebuild-wasmtarget that strips symbols, improves reproducibility, and runs a dual-passwasm-optoptimization.Protocol serialization and runtime performance are tightened:
rdsec/rdverbgenerated protos are replaced with reflection-free variants and handshake code switches fromproto.Marshal/UnmarshaltoMarshalVT/UnmarshalVT; several hot-pathsync.Poolbuffers (tunnel, rate limiters, BPS manager) now pool*[]byteto reduce allocations;WsStreamis refactored to be testable (interface-backed conn) and to handle multi-frame reads correctly, with extensive new tests/benchmarks added for protobuf VT methods, rate limiting, wsstream, and utilities. Tooling is also updated via pre-commit config/justfile, and dependencies are bumped ingo.mod/go.sum.Written by Cursor Bugbot for commit f93cd82. This will update automatically on new commits. Configure here.