OSAC-3544: Report WebSocket console errors as close frames - #138
Conversation
|
@sk-ilya: This pull request references OSAC-3544 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe console WebSocket flow now uses callback-scoped contexts, post-upgrade close statuses for setup failures, managed connection timeouts, and explicit session-establishment tracking for metrics. ChangesWebSocket session flow
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant WebSocketClient
participant ConsoleProxyWSHandler
participant BackendConnection
WebSocketClient->>ConsoleProxyWSHandler: WebSocket upgrade
ConsoleProxyWSHandler->>ConsoleProxyWSHandler: verify ticket
ConsoleProxyWSHandler->>BackendConnection: connect backend
BackendConnection-->>ConsoleProxyWSHandler: backend result
ConsoleProxyWSHandler->>WebSocketClient: send close status on setup error
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@fulfillment-service/internal/servers/console_proxy_ws.go`:
- Around line 138-164: The connCtx created from context.WithoutCancel removes
the request deadline, and if OpenTicket or ConnectBackend blocks indefinitely,
the deferred cancel cannot provide timeout protection. Create a separate bounded
setup context with a deadline for the OpenTicket and ConnectBackend operations,
and establish the longer-lived connCtx only after setup succeeds. Update the
calls to OpenTicket and ConnectBackend to use the setup context instead of
connCtx, preserving connCtx for the backend connection phase that follows.
- Around line 85-92: The closeWithStatus function blocks indefinitely during the
ws.Close handshake, creating a resource exhaustion vector when authentication
fails. Wrap the ws.Close call with a timeout mechanism using a goroutine and
timer (since Close does not accept a context parameter). If the timer fires
before the Close handshake completes, invoke ws.CloseNow to immediately
terminate the connection rather than waiting for the full 10-second handshake.
Use a timeout duration of 1-2 seconds to balance graceful shutdown with DoS
prevention. Verify that concurrent calls to Close and CloseNow do not create
race conditions with the websocket.Conn implementation.
- Line 136: Handle the CloseNow errors in both cleanup paths: in
fulfillment-service/internal/servers/console_proxy_ws.go:136, log any cleanup
failure from the deferred CloseNow call using the surrounding server logging
mechanism; in fulfillment-service/internal/servers/console_proxy_ws_test.go:42,
assert or record the CloseNow result through the test’s existing error-handling
mechanism. Do not discard either return value.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2e8419a7-b537-4ea1-8176-22d640321832
📒 Files selected for processing (6)
fulfillment-service/internal/console/kubevirt_backend.gofulfillment-service/internal/console/ping.gofulfillment-service/internal/servers/console_http_middleware.gofulfillment-service/internal/servers/console_http_middleware_test.gofulfillment-service/internal/servers/console_proxy_ws.gofulfillment-service/internal/servers/console_proxy_ws_test.go
| ) | ||
| return | ||
| } | ||
| defer func() { _ = ws.CloseNow() }() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the files
find . -type f -name "console_proxy_ws.go" -o -name "console_proxy_ws_test.go" | head -20Repository: osac-project/osac
Length of output: 278
🏁 Script executed:
# Examine the code around line 136 in console_proxy_ws.go
cat -n fulfillment-service/internal/servers/console_proxy_ws.go | sed -n '130,145p'Repository: osac-project/osac
Length of output: 896
🏁 Script executed:
# Examine the code around line 42 in console_proxy_ws_test.go
cat -n fulfillment-service/internal/servers/console_proxy_ws_test.go | sed -n '35,55p'Repository: osac-project/osac
Length of output: 948
🏁 Script executed:
# Search for the CloseNow method definition to confirm its signature
rg -A 3 'func.*CloseNow' fulfillment-service/internal/servers/Repository: osac-project/osac
Length of output: 568
🏁 Script executed:
# Check what type ws is in both contexts
ast-grep outline fulfillment-service/internal/servers/console_proxy_ws.go | head -50Repository: osac-project/osac
Length of output: 1072
🏁 Script executed:
# Check the imports to understand which websocket package is being used
head -30 fulfillment-service/internal/servers/console_proxy_ws.go | grep -A 20 "^import"Repository: osac-project/osac
Length of output: 589
🏁 Script executed:
# Search for the websocket package and CloseNow signature in the codebase or documentation
rg "websocket" fulfillment-service/internal/servers/console_proxy_ws.go | head -20Repository: osac-project/osac
Length of output: 801
🏁 Script executed:
# Check if there's a go.mod to see the websocket dependency
cat go.mod | grep websocketRepository: osac-project/osac
Length of output: 194
🏁 Script executed:
# Check the go.mod in fulfillment-service directory
find . -name "go.mod" | xargs grep -l websocket 2>/dev/nullRepository: osac-project/osac
Length of output: 207
🏁 Script executed:
# Search for other uses of CloseNow in the codebase to see patterns
rg "CloseNow" . --type goRepository: osac-project/osac
Length of output: 2184
🏁 Script executed:
# Look for the closeWithStatus function to understand error handling patterns for WebSocket close
cat -n fulfillment-service/internal/servers/console_proxy_ws.go | sed -n '85,102p'Repository: osac-project/osac
Length of output: 963
🏁 Script executed:
# Look for how closeWithStatus function is used - it handles Close errors
rg -B 2 -A 2 "closeWithStatus" fulfillment-service/internal/servers/console_proxy_ws.goRepository: osac-project/osac
Length of output: 1181
🏁 Script executed:
# Search for the websocket.Conn type definition or documentation in go.mod
cat fulfillment-service/go.mod | grep -A 2 -B 2 websocketRepository: osac-project/osac
Length of output: 341
🏁 Script executed:
# Check if there are any error handlers for CloseNow in the broader codebase
rg -B 3 -A 3 "CloseNow.*err\|err.*CloseNow" . --type goRepository: osac-project/osac
Length of output: 155
🏁 Script executed:
# Look for the test file more carefully - especially around the defer statement
cat -n fulfillment-service/internal/servers/console_proxy_ws_test.go | sed -n '30,50p'Repository: osac-project/osac
Length of output: 1149
🌐 Web query:
coder/websocket CloseNow error return type documentation
💡 Result:
In the coder/websocket Go library, the CloseNow method has the signature: func (c *Conn) CloseNow (err error) The method returns an error type (standard Go error) [1][2]. Regarding the behavior and return values of CloseNow: 1. Initial/Success Case: When called for the first time on an active connection, it closes the WebSocket connection immediately without performing a close handshake [1][2]. It typically returns nil [3]. 2. Concurrent/Repeated Calls: If called while a close operation is already in progress, or on a connection that has already been closed, it may return a wrapped net.ErrClosed, depending on the specific timing and state [3][4]. 3. Handling Errors: Because it returns an error, callers should handle the potential for net.ErrClosed if they are wrapping the call or checking the returned value [3][4]. CloseNow is specifically intended for scenarios where you want to terminate the connection immediately without the overhead of the standard WebSocket close handshake [1][2].
Citations:
- 1: https://github.com/coder/websocket/blob/v1.8.13/close.go
- 2: https://ref.gotd.dev/pkg/github.com/coder/websocket.html
- 3: fix: suppress net.ErrClosed on concurrent Close coder/websocket#564
- 4: https://github.com/coder/websocket/blob/master/conn_test.go
🏁 Script executed:
# Let's verify if CloseNow returns an error by checking how it's declared
# Look for any documentation comments or type signatures in the codebase
rg "type Conn struct" fulfillment-service/ --type go -A 20Repository: osac-project/osac
Length of output: 155
🏁 Script executed:
# Check how errors are typically handled in other cleanup paths in the codebase
rg -B 2 -A 2 "defer.*Close\|defer.*close" fulfillment-service/internal/servers/ --type go | head -40Repository: osac-project/osac
Length of output: 155
🏁 Script executed:
# Examine more context to understand the function behavior and cleanup needs
cat -n fulfillment-service/internal/servers/console_proxy_ws.go | sed -n '104,200p'Repository: osac-project/osac
Length of output: 3944
🏁 Script executed:
# Check if there's any pattern in the test file that shows how to properly assert cleanup
cat -n fulfillment-service/internal/servers/console_proxy_ws_test.go | head -70Repository: osac-project/osac
Length of output: 3002
🏁 Script executed:
# Look at the broader test context to understand if cleanup errors are typically asserted
rg "DeferCleanup\|defer.*Expect\|Expect.*defer" fulfillment-service/internal/servers/console_proxy_ws_test.go -B 2 -A 2Repository: osac-project/osac
Length of output: 155
Handle all CloseNow error returns.
Both cleanup paths discard errors returned by CloseNow().
fulfillment-service/internal/servers/console_proxy_ws.go#L136: Log the cleanup failure to aid diagnosis of connection closure problems.fulfillment-service/internal/servers/console_proxy_ws_test.go#L42: Assert or record the test cleanup result.
Per Go practices, never ignore error returns.
📍 Affects 2 files
fulfillment-service/internal/servers/console_proxy_ws.go#L136-L136(this comment)fulfillment-service/internal/servers/console_proxy_ws_test.go#L42-L42
🤖 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 `@fulfillment-service/internal/servers/console_proxy_ws.go` at line 136, Handle
the CloseNow errors in both cleanup paths: in
fulfillment-service/internal/servers/console_proxy_ws.go:136, log any cleanup
failure from the deferred CloseNow call using the surrounding server logging
mechanism; in fulfillment-service/internal/servers/console_proxy_ws_test.go:42,
assert or record the CloseNow result through the test’s existing error-handling
mechanism. Do not discard either return value.
Source: Path instructions
2de3e2d to
ff44e6f
Compare
ff44e6f to
a0a65c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@fulfillment-service/internal/servers/console_http_middleware_test.go`:
- Line 92: Handle the return errors from both conn.Close and resp.Body.Close in
the affected test cleanup paths. Check each close result and report or
explicitly handle expected failures, ensuring no Go error return is discarded
while preserving the existing test behavior.
- Around line 88-92: Add defer GinkgoRecover() at the start of the
http.HandlerFunc passed to ConsoleMetrics, before the Hijack assertion, so
assertion failures from the net/http goroutine propagate to Ginkgo.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 052e2f63-5fb1-4f19-9411-bb38b87b090e
📒 Files selected for processing (6)
fulfillment-service/internal/console/kubevirt_backend.gofulfillment-service/internal/console/ping.gofulfillment-service/internal/servers/console_http_middleware.gofulfillment-service/internal/servers/console_http_middleware_test.gofulfillment-service/internal/servers/console_proxy_ws.gofulfillment-service/internal/servers/console_proxy_ws_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- fulfillment-service/internal/console/ping.go
- fulfillment-service/internal/servers/console_http_middleware.go
- fulfillment-service/internal/servers/console_proxy_ws_test.go
- fulfillment-service/internal/servers/console_proxy_ws.go
| w.WriteHeader(http.StatusSwitchingProtocols) | ||
| conn, _, err := w.(http.Hijacker).Hijack() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| conn.Close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle both close errors.
The test discards errors from conn.Close() and resp.Body.Close(). Check both returns or handle expected close failures explicitly.
Proposed fix
- conn.Close()
+ Expect(conn.Close()).To(Succeed())
...
- resp.Body.Close()
+ Expect(resp.Body.Close()).To(Succeed())As per path instructions, Go code must not ignore error returns.
Also applies to: 100-100
🤖 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 `@fulfillment-service/internal/servers/console_http_middleware_test.go` at line
92, Handle the return errors from both conn.Close and resp.Body.Close in the
affected test cleanup paths. Check each close result and report or explicitly
handle expected failures, ensuring no Go error return is discarded while
preserving the existing test behavior.
Source: Path instructions
|
/ok-to-test |
|
Labeled |
a0a65c2 to
0366ef2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/ok-to-test |
|
Labeled |
0366ef2 to
a1c667f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@fulfillment-service/internal/servers/console_http_middleware_test.go`:
- Line 67: Update the tests around setSessionEstablished to use Eventually when
asserting direct 101 metrics, accounting for asynchronous metric recording after
http.Get returns. In the mixed-session test, wait for both status counters;
assert duration is present for established sessions and absent for unestablished
sessions.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1a541206-be6c-4230-96b3-115981942ac6
📒 Files selected for processing (6)
fulfillment-service/internal/console/kubevirt_backend.gofulfillment-service/internal/console/ping.gofulfillment-service/internal/servers/console_http_middleware.gofulfillment-service/internal/servers/console_http_middleware_test.gofulfillment-service/internal/servers/console_proxy_ws.gofulfillment-service/internal/servers/console_proxy_ws_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- fulfillment-service/internal/console/kubevirt_backend.go
- fulfillment-service/internal/console/ping.go
- fulfillment-service/internal/servers/console_proxy_ws_test.go
- fulfillment-service/internal/servers/console_proxy_ws.go
| w.WriteHeader(http.StatusSwitchingProtocols) | ||
| conn, _, err := w.(http.Hijacker).Hijack() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| setSessionEstablished(r.Context()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="$(git ls-files 'fulfillment-service/internal/servers/console_http_middleware_test.go' | head -n1)"
test -n "$file"
printf '%s\n' "== file =="
printf '%s\n' "$file"
printf '%s\n' "== relevant test sections =="
sed -n '1,230p' "$file"
printf '%s\n' "== remaining relevant section =="
sed -n '230,290p' "$file"
printf '%s\n' "== metric and middleware references =="
rg -n -C 3 'ConsoleMetrics|setSessionEstablished|http\.Get|Eventually|101|Close\(' fulfillment-service/internal/serversRepository: osac-project/osac
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' "== middleware definitions =="
rg -l 'func ConsoleMetrics|type ConsoleMetrics|console_websocket_connection_duration_seconds' fulfillment-service/internal/servers fulfillment-service/internal/testing |
while IFS= read -r file; do
printf '\n-- %s --\n' "$file"
rg -n -C 12 'func ConsoleMetrics|console_websocket_connection_duration_seconds|active_connections|sessionEstablished|setSessionEstablished' "$file"
done
printf '%s\n' "== metrics server implementation =="
rg -l 'type MetricsServer|func .*MetricsServer|func \(.*Metrics\(' fulfillment-service/internal/testing |
while IFS= read -r file; do
printf '\n-- %s --\n' "$file"
rg -n -C 12 'type MetricsServer|func .*MetricsServer|func \(.*Metrics\(' "$file"
done
printf '%s\n' "== relevant module/runtime metadata =="
rg -n 'go [0-9]+\.[0-9]+|prometheus|ginkgo|gomega' go.mod fulfillment-service/go.mod 2>/dev/null || trueRepository: osac-project/osac
Length of output: 14219
🏁 Script executed:
#!/bin/bash
set -eu
file=fulfillment-service/internal/servers/console_http_middleware.go
printf '%s\n' "== middleware implementation =="
sed -n '120,205p' "$file"
printf '%s\n' "== test diff summary and changed lines =="
git diff --stat -- "$file" fulfillment-service/internal/servers/console_http_middleware_test.go
git diff --unified=8 -- "$file" fulfillment-service/internal/servers/console_http_middleware_test.go
printf '%s\n' "== Go server hijack and response completion sources =="
goroot="$(go env GOROOT 2>/dev/null || true)"
if [ -n "$goroot" ] && [ -f "$goroot/src/net/http/server.go" ]; then
rg -n -C 8 'func \(c \*conn\) serve|hijackLocked|ServeHTTP|handler\.ServeHTTP|wroteHeader' "$goroot/src/net/http/server.go" | head -n 260
else
printf '%s\n' "Go source tree is unavailable"
fiRepository: osac-project/osac
Length of output: 12158
🏁 Script executed:
#!/bin/bash
set -eu
goroot="$(go env GOROOT)"
printf '%s\n' "== request serving and handler return order =="
rg -n -C 18 'serverHandler\{c\}\.ServeHTTP|handler\.ServeHTTP|func \(c \*conn\) serve|if inFlightResponse|hijackedv' "$goroot/src/net/http/server.go" | head -n 360
printf '%s\n' "== standalone 101 timing probe =="
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/main.go" <<'EOF'
package main
import (
"bufio"
"fmt"
"net"
"net/http"
"net/http/httptest"
"strings"
"sync"
"time"
)
func main() {
handlerDone := make(chan struct{})
handlerHijacked := make(chan struct{})
release := make(chan struct{})
var once sync.Once
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusSwitchingProtocols)
conn, _, err := w.(http.Hijacker).Hijack()
if err != nil {
panic(err)
}
once.Do(func() { close(handlerHijacked) })
<-release
conn.Close()
close(handlerDone)
}))
defer srv.Close()
addr := strings.TrimPrefix(srv.URL, "http://")
conn, err := net.Dial("tcp", addr)
if err != nil {
panic(err)
}
defer conn.Close()
_, err = fmt.Fprintf(conn, "GET / HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", addr)
if err != nil {
panic(err)
}
headers, err := http.ReadResponse(bufio.NewReader(conn), nil)
if err != nil {
panic(err)
}
if headers.StatusCode != http.StatusSwitchingProtocols {
panic(headers.Status)
}
select {
case <-handlerHijacked:
case <-time.After(2 * time.Second):
panic("handler did not hijack")
}
select {
case <-handlerDone:
panic("handler completed before client received 101")
default:
fmt.Println("client received 101 while handler and middleware could still be running")
}
close(release)
select {
case <-handlerDone:
case <-time.After(2 * time.Second):
panic("handler did not complete")
}
}
EOF
(cd "$tmpdir" && go run main.go)Repository: osac-project/osac
Length of output: 6336
🏁 Script executed:
#!/bin/bash
set -eu
goroot="$(go env GOROOT)"
printf '%s\n' "== handler invocation and post-handler flush =="
sed -n '2030,2145p' "$goroot/src/net/http/server.go"
printf '%s\n' "== standalone 101 timing probe =="
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/main.go" <<'EOF'
package main
import (
"bufio"
"fmt"
"net"
"net/http"
"net/http/httptest"
"strings"
"sync"
"time"
)
func main() {
handlerDone := make(chan struct{})
handlerHijacked := make(chan struct{})
release := make(chan struct{})
var once sync.Once
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusSwitchingProtocols)
conn, _, err := w.(http.Hijacker).Hijack()
if err != nil {
panic(err)
}
once.Do(func() { close(handlerHijacked) })
<-release
conn.Close()
close(handlerDone)
}))
defer srv.Close()
addr := strings.TrimPrefix(srv.URL, "http://")
conn, err := net.Dial("tcp", addr)
if err != nil {
panic(err)
}
defer conn.Close()
if _, err = fmt.Fprintf(conn, "GET / HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", addr); err != nil {
panic(err)
}
response, err := http.ReadResponse(bufio.NewReader(conn), nil)
if err != nil {
panic(err)
}
if response.StatusCode != http.StatusSwitchingProtocols {
panic(response.Status)
}
select {
case <-handlerHijacked:
case <-time.After(2 * time.Second):
panic("handler did not hijack")
}
select {
case <-handlerDone:
panic("handler completed before client received 101")
default:
fmt.Println("client received 101 while the handler was still running")
}
close(release)
select {
case <-handlerDone:
case <-time.After(2 * time.Second):
panic("handler did not complete")
}
}
EOF
(cd "$tmpdir" && CGO_ENABLED=0 go run main.go)Repository: osac-project/osac
Length of output: 3756
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' "== standalone 101 timing probe =="
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/main.go" <<'EOF'
package main
import (
"bufio"
"fmt"
"net"
"net/http"
"strings"
"time"
)
func main() {
handlerDone := make(chan struct{})
handlerHijacked := make(chan struct{})
release := make(chan struct{})
server := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusSwitchingProtocols)
conn, _, err := w.(http.Hijacker).Hijack()
if err != nil {
panic(err)
}
close(handlerHijacked)
<-release
conn.Close()
close(handlerDone)
})}
listener, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
panic(err)
}
defer listener.Close()
go func() {
if err := server.Serve(listener); err != nil && err != http.ErrServerClosed {
panic(err)
}
}()
addr := listener.Addr().String()
conn, err := net.Dial("tcp", addr)
if err != nil {
panic(err)
}
defer conn.Close()
if _, err = fmt.Fprintf(conn, "GET / HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", addr); err != nil {
panic(err)
}
response, err := http.ReadResponse(bufio.NewReader(conn), nil)
if err != nil {
panic(err)
}
if response.StatusCode != http.StatusSwitchingProtocols {
panic(response.Status)
}
select {
case <-handlerHijacked:
case <-time.After(2 * time.Second):
panic("handler did not hijack")
}
select {
case <-handlerDone:
panic("handler completed before client received 101")
default:
fmt.Println("client received 101 while the handler was still running")
}
close(release)
select {
case <-handlerDone:
case <-time.After(2 * time.Second):
panic("handler did not complete")
}
_ = strings.TrimSpace(response.Status)
}
EOF
(cd "$tmpdir" && CGO_ENABLED=0 go run main.go)Repository: osac-project/osac
Length of output: 339
Synchronize direct 101 metric assertions.
http.Get can return after the 101 headers arrive but before ConsoleMetrics.ServeHTTP records the metrics. Use Eventually for the expected counters. Wait for both status counters in the mixed test. Assert duration presence for established sessions and absence for unestablished sessions.
🤖 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 `@fulfillment-service/internal/servers/console_http_middleware_test.go` at line
67, Update the tests around setSessionEstablished to use Eventually when
asserting direct 101 metrics, accounting for asynchronous metric recording after
http.Get returns. In the mixed-session test, wait for both status counters;
assert duration is present for established sessions and absent for unestablished
sessions.
|
Removed |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
fulfillment-service/internal/servers/console_http_middleware_test.go (2)
94-94: 🩺 Stability & Availability | 🟡 MinorHandle close errors in the new test.
net.Conn.Close()andresp.Body.Close()return errors, but this test discards both. Check each return or explicitly handle the expected close behavior.Proposed fix
- conn.Close() + Expect(conn.Close()).To(Succeed()) ... - resp.Body.Close() + Expect(resp.Body.Close()).To(Succeed())As per path instructions, Go code must not ignore error returns.
Also applies to: 102-102
🤖 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 `@fulfillment-service/internal/servers/console_http_middleware_test.go` at line 94, Update the new test’s cleanup at both conn.Close and resp.Body.Close to explicitly handle their returned errors, using the test’s existing error-reporting mechanism or an intentional assertion for the expected close behavior; do not discard either return value.Source: Path instructions
62-68: 🩺 Stability & Availability | 🟡 MinorWait for middleware metrics before asserting.
http.Getcan return after the101headers arrive while the handler andConsoleMetricsare still finishing. The immediatemetricsServer.Metrics()calls can read an incomplete snapshot. UseEventuallyfor positive status and duration assertions. In the unestablished case, wait for the error counter before asserting that no duration metric exists. In the mixed case, wait for both status counters. This repeats the previous review finding and remains unresolved.Also applies to: 85-107, 110-121, 193-198
🤖 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 `@fulfillment-service/internal/servers/console_http_middleware_test.go` around lines 62 - 68, Update the assertions in the affected ConsoleMetrics tests, including the 101 Switching Protocols, unestablished, and mixed-status cases, to wait for middleware metric processing with Eventually before reading metrics. Wait for positive status and duration metrics, wait for the error counter before asserting no duration metric in the unestablished case, and wait for both status counters in the mixed case.
🤖 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.
Duplicate comments:
In `@fulfillment-service/internal/servers/console_http_middleware_test.go`:
- Line 94: Update the new test’s cleanup at both conn.Close and resp.Body.Close
to explicitly handle their returned errors, using the test’s existing
error-reporting mechanism or an intentional assertion for the expected close
behavior; do not discard either return value.
- Around line 62-68: Update the assertions in the affected ConsoleMetrics tests,
including the 101 Switching Protocols, unestablished, and mixed-status cases, to
wait for middleware metric processing with Eventually before reading metrics.
Wait for positive status and duration metrics, wait for the error counter before
asserting no duration metric in the unestablished case, and wait for both status
counters in the mixed case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 768910a0-e25a-4c25-960f-f8b48dbf3409
📒 Files selected for processing (1)
fulfillment-service/internal/servers/console_http_middleware_test.go
Browsers collapse non-101 handshake responses into a generic error/1006, making error codes invisible to JS. Move setup error reporting (bad ticket, session conflict, backend failure) to post-upgrade close frames so browser clients can read the code and reason from CloseEvent. Assisted-by: Claude <noreply@anthropic.com>
a1c667f to
47c683f
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omer-vishlitzky, sk-ilya, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Summary
Browsers collapse non-101 WebSocket handshake responses into a generic error event with close code 1006, making error details invisible to JavaScript. This moves setup error reporting to post-upgrade close frames so browser clients can read the actual error code and reason from CloseEvent.
Bad ticket and expired ticket close with 3000 (IANA-registered "Unauthorized"), session conflict with 4409 (private-use, HTTP 409 equivalent), and backend failure with 1014 (StatusBadGateway). The metrics middleware is updated to distinguish successful sessions from upgraded-but-rejected connections using an explicit session-established signal instead of HTTP status code inspection.
Companion test update: osac-project/osac-test-infra#313
Test plan
Unit tests cover close code delivery, metrics labeling, origin enforcement, and ticket extraction. Run
ginkgo run -r internalanduv run dev.py lint go.Assisted-by: Claude noreply@anthropic.com
Summary by CodeRabbit