Skip to content

Conversation

@garmr-ulfr
Copy link
Collaborator

This pull request refactors the client context tracker in tracker/clientcontext/tracker.go to use standard HTTP for client info exchange on stream connections and introduces a clear packet prefix for client info on packet connections. It also updates the integration tests to reflect the new handshake logic and improves test coverage for tracker usage.

Protocol and handshake changes:

  • Stream connections now exchange client info using a standard HTTP POST request to /clientinfo, and expect an HTTP 200 OK response, replacing the previous plain-text protocol. (tracker/clientcontext/tracker.go) [1] [2]
  • Packet connections now send client info as a packet prefixed with CLIENTINFO , and the server responds with "OK" as before, but with improved error handling and context management. (tracker/clientcontext/tracker.go) [1] [2]

Code structure and error handling improvements:

  • Refactored connection wrappers (readConn, writeConn, readPacketConn, writePacketConn) to improve error handling, context propagation, and to cache initial reads for protocol detection. (tracker/clientcontext/tracker.go) [1] [2] [3]
  • Enhanced logging: errors during client info reading now log warnings instead of errors, and the connection proceeds gracefully if client info is not present. (tracker/clientcontext/tracker.go) [1] [2]

Testing improvements:

  • Integration tests now verify both the presence and absence of the client context tracker, ensuring correct behavior in both scenarios. The test setup is refactored for clarity and correctness. (tracker/clientcontext/tracker_test.go)
  • The test utility newTestBox now only attaches a tracker if one is provided, making test setup more flexible. (tracker/clientcontext/tracker_test.go)

Dependency and import updates:

  • Added necessary imports for HTTP and buffer handling, such as net/http, io, and bytes, to support the new handshake logic. (tracker/clientcontext/tracker.go)

Copilot AI review requested due to automatic review settings December 16, 2025 01:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the client context tracker to use standard HTTP for stream connections and introduces a CLIENTINFO prefix for packet connections, improving protocol clarity and error handling.

Key changes:

  • Stream connections now use HTTP POST requests to /clientinfo with HTTP 200 OK responses instead of plain-text protocol
  • Packet connections prefix client info with CLIENTINFO for better protocol detection
  • Enhanced error handling with graceful fallback when client info is absent, logging warnings instead of errors

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
tracker/clientcontext/tracker.go Implements HTTP-based handshake for stream connections, adds CLIENTINFO prefix for packets, and improves error handling with buffered reads for protocol detection
tracker/clientcontext/tracker_test.go Adds test coverage for both presence and absence of client context tracker, refactors test setup to conditionally attach trackers

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +202 to +210
var buf [32]byte
n, err := c.Conn.Read(buf[:])
if err != nil {
c.readErr = err
c.n = n
return err
}
reader := io.MultiReader(bytes.NewReader(buf[:n]), c.Conn)
if !bytes.HasPrefix(buf[:n], []byte("POST /clientinfo")) {
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The protocol detection only checks the first n bytes read (up to 32 bytes), which may not contain the full prefix 'POST /clientinfo' if the network delivers fewer bytes initially. This could cause incorrect protocol detection. Consider reading enough bytes to ensure the full HTTP method and path can be checked, or use a more robust detection mechanism.

Suggested change
var buf [32]byte
n, err := c.Conn.Read(buf[:])
if err != nil {
c.readErr = err
c.n = n
return err
}
reader := io.MultiReader(bytes.NewReader(buf[:n]), c.Conn)
if !bytes.HasPrefix(buf[:n], []byte("POST /clientinfo")) {
prefix := []byte("POST /clientinfo")
buf := make([]byte, 0, 64) // start with a reasonable buffer size
tmp := make([]byte, 32)
var n int
var err error
for len(buf) < len(prefix) {
n, err = c.Conn.Read(tmp)
if n > 0 {
buf = append(buf, tmp[:n]...)
}
if err != nil {
if err != io.EOF || len(buf) < len(prefix) {
c.readErr = err
c.n = len(buf)
return err
}
break
}
}
reader := io.MultiReader(bytes.NewReader(buf), c.Conn)
if len(buf) < len(prefix) || !bytes.HasPrefix(buf, prefix) {

Copilot uses AI. Check for mistakes.
@garmr-ulfr garmr-ulfr merged commit bc31f24 into main Dec 17, 2025
1 check passed
@garmr-ulfr garmr-ulfr deleted the detect-context-tracker branch December 17, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants