Shutdown Websocket Connections Gracefully in the queue-proxy by dprotaso · Pull Request #16362 · knative/serving

dprotaso · 2026-01-27T21:44:58Z

have the websocket server test image send a close message when shutting down gracefully
include an e2e test that asserts websockets connections are gracefully handled
include a special handler to track hijacked connections and create a drain method

knative-prow · 2026-01-27T21:45:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2026-01-27T21:50:58Z

Codecov Report

❌ Patch coverage is 41.66667% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.22%. Comparing base (2cd582b) to head (1e11ed1).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/queue/sharedmain/handlers.go	0.00%	11 Missing ⚠️
pkg/queue/sharedmain/main.go	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16362      +/-   ##
==========================================
+ Coverage   80.18%   80.22%   +0.03%     
==========================================
  Files         216      217       +1     
  Lines       13440    13507      +67     
==========================================
+ Hits        10777    10836      +59     
- Misses       2298     2306       +8     
  Partials      365      365

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dprotaso · 2026-01-27T21:56:37Z

One observation I have is that the activator will drop hijacked connections similar to the queue proxy.

Hijacking happens implicitly with the httputil.ReverseProxy - https://cs.opensource.google/go/go/+/refs/tags/go1.25.6:src/net/http/httputil/reverseproxy.go;l=504

The solution in the queue-proxy is straight forward because we know K8s will send a TERM signal to the user container - thus we can expect apps to return a close frame when that happens. Those are the changes I made to our websocket server. This isn't necessary but just maybe it's a good practice

With the activator though - there's no way to signal downstream to end the connection. We maybe could send a close frame on the connection but it makes the activator protocol aware and I don't think we can write a message on an upstream connection like this (it could be interleaved with another).

Given this - I don't think the activator issue should prevent this change from merging - but just the solution there isn't straighforward.

dprotaso · 2026-01-27T23:23:26Z

/retest

hmm

websocket_test.go:162: Successfully wrote: "Hello, websocket"
websocket_test.go:190: error running test failed to read message: read tcp 10.250.69.110:60956->34.138.223.27:80: read: connection reset by peer

dprotaso · 2026-02-06T04:26:43Z

/retest

… the queue-proxy

dprotaso · 2026-02-06T23:39:25Z

.github/workflows/kind-e2e.yaml

        gotestsum --format testname -- \
          -race -count=1 -parallel=1 -tags=e2e \
-          -timeout=30m \
+          -timeout=35m \


With the extra test we need to bump this since we run things with -parallel=1 due to github actions default runners being crappy.

I adjusted the test to not use time.Sleep to speed it up

Maybe not that relevant for this PR but timing tests could also be sped up using synctest after requiring go 1.25, see https://go.dev/blog/testing-time

Not for e2e. It would run faster if we ran tests in parallel - but with kind+github action runners they are so under provisioned that tests flake out etc.

…irst message

dprotaso · 2026-02-07T02:23:06Z

/assign @linkvt @elijah-rou

knative-prow · 2026-02-07T02:23:09Z

@dprotaso: GitHub didn't allow me to assign the following users: elijah-rou.

Note that only knative members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

Details

In response to this:

/assign @linkvt @elijah-rou

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 kubernetes-sigs/prow repository.

linkvt

Looks good to me in general, took me a while to figure out the concurrency and what is actually tested, I left a few minor comments though.

test/e2e/websocket_test.go

pkg/http/handler/hijack.go

linkvt · 2026-02-09T12:53:34Z

pkg/http/handler/hijack_test.go

+		<-inHandler
+		close(drainStarted)
+		drainResult <- h.Drain(context.Background())
+	}()
+
+	<-drainStarted
+	close(handlerWait)


I'm not sure if this always behaves as expected, couldn't a potential sequence be:

B: inHandler is unblocked

B: close drainStarted

Test: drainStarted is unblocked

Test: close handlerWait

A: handlerWait is unblocked, returns and inflight requests equals 0

B: nothing to Drain

Should this test verify that drain works correctly when a connection is closed after the tracker was asked to Drain?

You think I should add a sleep in-between drainStarted being unblocked and closing handlerWait?

I think I was thinking that close(drainStarted) wouldn't yield the go-routine so the drain does start but I don't believe there is a guarantee like that in Go

Yes I think we can remove drainStarted, move <-inHandler out of the second goroutine between the two goroutines and add a sleep before close(handlerWait).

This way the handler would start, unblock <-inHandler, start the goroutine to drain, sleep in the test func so that Drain will run the loop a few times before close(handlerWait) lets the handler finish.

Edit: We could even add a select to check whether <-drainResult has received a value before this sleep passes.

done - take a look

linkvt · 2026-02-10T09:16:44Z

pkg/http/handler/hijack_test.go

+		<-inHandler
+		close(drainStarted)
+		drainResult <- h.Drain(context.Background())
+	}()
+
+	<-drainStarted
+	close(handlerWait)


Yes I think we can remove drainStarted, move <-inHandler out of the second goroutine between the two goroutines and add a sleep before close(handlerWait).

This way the handler would start, unblock <-inHandler, start the goroutine to drain, sleep in the test func so that Drain will run the loop a few times before close(handlerWait) lets the handler finish.

Edit: We could even add a select to check whether <-drainResult has received a value before this sleep passes.

pkg/http/handler/hijack.go

linkvt · 2026-02-10T17:17:27Z

/lgtm

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 27, 2026

dprotaso changed the title ~~Shutdown Websocket Connections Gracefully~~ [wip] Shutdown Websocket Connections Gracefully Jan 27, 2026

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 27, 2026

knative-prow bot requested review from dsimansk, gauron99 and skonto January 27, 2026 21:45

dprotaso mentioned this pull request Jan 27, 2026

Ensure conections persist until done on queue-proxy drain #16080

Closed

dprotaso changed the title ~~[wip] Shutdown Websocket Connections Gracefully~~ [wip] Shutdown Websocket Connections Gracefully in the queue-proxy Jan 27, 2026

dprotaso force-pushed the websocket-graceful branch 2 times, most recently from 9f80c61 to cd8e486 Compare February 5, 2026 02:46

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2026

dprotaso force-pushed the websocket-graceful branch from b889e35 to b434a0a Compare February 5, 2026 03:14

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2026

dprotaso added 3 commits February 6, 2026 12:22

include new hijack handler to help drain hijacked requests by polling

81dc57d

Update echo websocket server to gracefully exit on term signal

4845575

include new test to ensure websockets connections gracefully exit via…

55df508

… the queue-proxy

dprotaso force-pushed the websocket-graceful branch from 411fe17 to 55df508 Compare February 6, 2026 17:34

increase test timeout to 35 min

19e3e0a

dprotaso changed the title ~~[wip] Shutdown Websocket Connections Gracefully in the queue-proxy~~ Shutdown Websocket Connections Gracefully in the queue-proxy Feb 6, 2026

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2026

dprotaso commented Feb 6, 2026

View reviewed changes

dprotaso added 2 commits February 6, 2026 18:42

fix copyright date of new files

00f0b3c

don't use sleeps but delete the service after we write and read the f…

0d5c031

…irst message

knative-prow bot assigned linkvt Feb 7, 2026

linkvt reviewed Feb 9, 2026

View reviewed changes

dprotaso added 2 commits February 9, 2026 20:12

send correct message in e2e websocket test

6b068a0

add godoc

63019ba

linkvt reviewed Feb 10, 2026

View reviewed changes

dprotaso added 2 commits February 10, 2026 11:48

drop use of an extra channel

2baffbf

fix typo

1e11ed1

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2026

knative-prow bot merged commit ce8ae6c into knative:main Feb 10, 2026
90 checks passed

dprotaso deleted the websocket-graceful branch February 10, 2026 19:03

Conversation

dprotaso commented Jan 27, 2026

Uh oh!

knative-prow bot commented Jan 27, 2026

Uh oh!

codecov bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dprotaso commented Jan 27, 2026

Uh oh!

dprotaso commented Jan 27, 2026

Uh oh!

dprotaso commented Feb 6, 2026

Uh oh!

dprotaso Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

dprotaso Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linkvt Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

dprotaso Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Feb 7, 2026

Uh oh!

knative-prow bot commented Feb 7, 2026

Uh oh!

linkvt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

linkvt Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

dprotaso Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

dprotaso Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

linkvt Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

linkvt Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

linkvt commented Feb 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 27, 2026 •

edited

Loading

dprotaso Feb 7, 2026 •

edited

Loading

linkvt Feb 10, 2026 •

edited

Loading

linkvt Feb 10, 2026 •

edited

Loading