Skip to content

Follow-ups from #723: otel traceparent clone cost, SSE OnConnect and session c.Set views, stale #714/#718 scope, ENOMEM claim wording, deliberate-red record #740

Description

@FumingPower3925

Follow-ups from PR #723 (Fixes #714, #717, #718), deferred under the maintainer's two-round review cap (2026-09-27). Round 2 of the independent review approved #723 at c98f068 with minors only; they are recorded here. Each was checked against c98f068.

  1. otel carrier: the traceparent clone buys nothing, and the doc names too narrow a reason. headerCarrier.Get (middleware/otel/carrier.go:37) clones every propagation header that is present, including traceparent. The W3C TraceContext propagator hex-decodes traceparent into fixed-size TraceID and SpanID arrays and keeps no substring, so that copy is a per-request allocation with no benefit: the PR's Cost table shows the traced request going from 16 allocs / 2313 B to 17 allocs / 2377 B, and traceparent + tracestate + baggage from 27 to 30 allocs. The clone is justified for tracestate and baggage, and more broadly than the doc comment (carrier.go:22-31) says: the SDK samplers copy the parent's TraceState into the server span and the batch processor exports it after the request ends, so a view in tracestate outlives every sampled request, not only detached streams. Either skip the clone for traceparent, or keep it and say in the doc that it is deliberate; fix the doc either way. Keys() also clones each name separately (carrier.go:57); one buffer, as cloneHeaderPairs does, would make it one allocation per call.

  2. SSE Config.OnConnect hands out views that a detached stream keeps. OnConnect is documented as the place to extract request metadata (middleware/sse/config.go:102-105) and runs before Detach (middleware/sse/sse.go:373 vs :426). A string a user keeps from it (a hub subscription keyed by c.Param or c.Query, a user ID stored for the stream) is a view the stream keeps for its whole life: middleware/websocket: on epoll and io_uring, Conn.Query returns garbage once a frame has arrived, because the captured query strings alias the engine's read buffer (200/200 per engine) #714's mechanism, reached through an in-tree API. The new Detach doc says strings read before the call cannot be copied, but an SSE user cannot move the read after Detach, because the middleware calls Detach itself. Fix by warning in OnConnect's doc that kept values must be cloned, or by materializing the request views before onConnect runs. The fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723 audit did not list this callback.

  3. Session middleware stores cookie views with c.Set. The audit's sweep covered first-party SetContext/WithValue, not first-party c.Set values, which the new Detach doc says the storer must clone. middleware/session/session.go:763/:765 set sess.id and sess.presentedID to the cookie view, and :784 stores sess with c.Set(ContextKey, sess). A detached SSE OnDisconnect(c, client), or any detached goroutine calling session.FromContext(c).ID() or Save, reads the reused buffer on epoll and io_uring. This is the session class of middleware/session: with WriteBehind on epoll and io_uring, a loaded session is saved under the NEXT request's session ID (the queued id is a view of the receive buffer): cross-session write #731; check whether its fix covers these two fields, and add the row to the audit next to the jwt Token.Raw note.

  4. middleware/websocket: on epoll and io_uring, Conn.Query returns garbage once a frame has arrived, because the captured query strings alias the engine's read buffer (200/200 per engine) #714, middleware/sse: on epoll and io_uring, Client.LastEventID returns other bytes once the connection has received more, because the header is kept as a view of the read buffer (20/20 per native cell) #717 and Context.Detach clones the headers, method, path and raw query but not the route params, query and cookie caches, Host, or middleware-set strings, so a detached Context reads other bytes on epoll and io_uring (7 fields, 20/20) #718 do not record what fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723 finally fixed. Round 2 of fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723 fixed four more defects under the existing issues: the response headers set before Detach and the request body (both under Context.Detach clones the headers, method, path and raw query but not the route params, query and cookie caches, Host, or middleware-set strings, so a detached Context reads other bytes on epoll and io_uring (7 fields, 20/20) #718), and the requestid EnableStdContext value and the otel carrier values (both under middleware/websocket: on epoll and io_uring, Conn.Query returns garbage once a frame has arrived, because the captured query strings alias the engine's read buffer (200/200 per engine) #714). Context.Detach clones the headers, method, path and raw query but not the route params, query and cookie caches, Host, or middleware-set strings, so a detached Context reads other bytes on epoll and io_uring (7 fields, 20/20) #718's title still says "7 fields" and names neither; middleware/websocket: on epoll and io_uring, Conn.Query returns garbage once a frame has arrived, because the captured query strings alias the engine's read buffer (200/200 per engine) #714's title is only about websocket Conn.Query, and the requestid and otel changes are per request and affect any code that keeps c.Context(), not only streams. None of the three issues has a comment. Add a comment to middleware/websocket: on epoll and io_uring, Conn.Query returns garbage once a frame has arrived, because the captured query strings alias the engine's read buffer (200/200 per engine) #714 and Context.Detach clones the headers, method, path and raw query but not the route params, query and cookie caches, Host, or middleware-set strings, so a detached Context reads other bytes on epoll and io_uring (7 fields, 20/20) #718 recording the expanded scope with the T/R1/H counts from fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723's Failing-first table, so the closed issues and the release notes carry it.

  5. The PR body's ENOMEM sentence is broader than its evidence. fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723's CI section says "No ENOMEM retry fired in this run, the two red runs, or any local run." The counted runs and the three CI Unit jobs are clean (0 server start retried lines; every io_uring listener reported tier=high), but the server-start ENOMEM retry did fire, 3 to 15 tries, in about 12 superseded local mutant runs. The probe retry (c714ProbeIOUring, detach_capture_alias_linux_test.go:323, and the same helper in the ws, sse and otel tests) never logs, so no log can show whether it fired, locally or in CI. Make the probe retry t.Logf when it fires, so CI can show it; the sentence should have been limited to the counted runs.

  6. Deliberately red CI runs, for the record. The branch is red-first: CI runs 36320632412 (9a89e0c) and 36321102421 (50825c1) are red on purpose (the tests before the fixes) and pair with the green run 36321745156 (c98f068). They belong in the maintainers' deliberate-red runs ledger. Because several commits on the branch fail CI on their own, fix: clone the request values a detached stream keeps (websocket Conn.Query, SSE Last-Event-ID, Context.Detach, requestid and otel context values) (#714, #717, #718) #723 must be squash-merged, never rebase-merged.

Not carried over: round 2 also asked whether #723's public body should keep the sentence about the multishot receive mode reading another connection's bytes. The maintainer chose public handling of that class (#731, #732, #733), so it stays.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    middlewareMiddleware implementation

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions