[mslynch]

Fixes #16169

When a websocket is proxied through queue-proxy (or the activator), httputil.ReverseProxy hijacks the connection and writes the 101 Switching Protocols response and every subsequent frame directly to the hijacked net.Conn/bufio.ReadWriter, bypassing timeoutWriter.Write/WriteHeader. lastWriteTime therefore stays zero for the life of the websocket, so:

• the response start timeout fires even when the app is actively sending frames, and
• the idle timeout fires even when the app is actively sending frames.

When either fires, ServeHTTP returns, the request context is cancelled, and ReverseProxy closes the backend connection, terminating an active websocket. With the default revision-response-start-timeout-seconds of 300, every websocket is cut at exactly 5 minutes regardless of traffic. queue-proxy also logs http: response.WriteHeader on hijacked connection as it tries to write the 504.

Proposed Changes

• As suggested in If connection is Hijack()ed, suspend first byte timeout #16171 (comment), Hijack returns a wrapped net.Conn whose writes update lastWriteTime, so bytes sent by the app after the upgrade count towards the response start and idle timeouts. As for regular HTTP responses, only bytes from the app count towards the idle timeout.
• Don't attempt to write a 504 to a hijacked connection when a timeout fires. The connection is still closed via context cancellation. Hijacking after the handler has already timed out returns http.ErrHandlerTimeout.
• Tests:
  • Unit tests proxying a websocket through NewTimeoutHandler + ReverseProxy: active connections outlive the response start and idle timeouts (both fail on main), and silent connections are still closed by both timeouts.
  • New e2e test TestWebSocketActiveConnectionOutlivesTimeouts: exchanges a message every second for 30s against revisions with a 10s response start / idle timeout. Fails on main at ~12s with close 1006 (abnormal closure); passes with this change.
  • No existing test expectations change. timeoutSeconds still bounds the total lifetime of the websocket.

Validated on a kind cluster with Kourier: all TestWebSocket* e2e tests pass.

Decision needed: does the 101 count as the response starting?

This PR currently does not count the 101 Switching Protocols response towards the response start timeout. ReverseProxy writes the 101 to the hijacked bufio.ReadWriter, which is left unwrapped, so the response start timeout measures the time until the app's first bytes after the upgrade. That keeps the existing TestWebSocketWithTimeout case "exceeds response start timeout seconds" passing: the server upgrades, then waits 20s before echoing, and the connection is closed by the 10s response start timeout.

The alternative is to also point the buffered writer at the wrapped connection (rw.Writer.Reset(wrappedConn)), so the upgrade response itself starts the response. Arguments for it:

• The API docs describe responseStartTimeoutSeconds as how long to wait for the container "to begin sending any network traffic", and the 101 comes from the container.
• Websockets are disconnected after ResponseStartTimeoutSeconds #16169 (comment) mentions "(or perform the handshake)" as satisfying this timeout.

With that alternative, a websocket whose app is silent after the handshake would only be bounded by the idle timeout and timeoutSeconds. The existing "exceeds response start timeout seconds" e2e case would need to flip to expect success. Either choice fixes active websockets being closed; this only affects websockets that are silent after the upgrade. Happy to switch to whichever behaviour maintainers prefer.

Release Note

Fixed websockets being closed by queue-proxy and the activator after the revision's responseStartTimeoutSeconds or idleTimeoutSeconds elapsed, even while the app was sending messages.

🤖 Generated with Claude Code

[knative-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mslynch
Once this PR has been reviewed and has the lgtm label, please assign dprotaso for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[linux-foundation-easycla]

• ❌ - login: @mslynch / name: Matthew Lynch. The commit (621e3be, a5ae823) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please visit our EasyCLA portal and chat with our support bot.

[knative-prow]

Welcome @mslynch! It looks like this is your first PR to knative/serving 🎉

[knative-prow]

Hi @mslynch. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.