Skip to content

feat: handle client_session_id in agent middleware - #28039

Merged
aqandrew merged 4 commits into
mainfrom
devex-660-session-id-agent-middleware
Aug 27, 2026
Merged

aqandrew merged 4 commits into
mainfrom
devex-660-session-id-agent-middleware

Conversation

@aqandrew

@aqandrew aqandrew commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Implements DEVEX-660: handle the client session ID in the agent middleware, per connection-log RFC requirement 6.2.

Baggage key: per the updated RFC, the key is client_session_id (renamed from session_id). The shared constant tracing.SessionIDBaggageKey now has the value client_session_id.

What

  • Add tracing.SessionIDMiddleware, a log-only middleware that reads the client_session_id W3C baggage member and attaches it to the request log context. Unlike tracing.Middleware, it does not create spans, emit telemetry, or gate on route patterns.
  • Wire it into the agent HTTP stack (agent/api.go) before loggermw.Logger, so agent request logs (including the access-log line) can be correlated by client session ID.

Why not spans on the agent

RFC 6.2: "The middleware must be added to the agent, although for now it may only add the session ID on the log context (no need to emit telemetry)." Spans/telemetry on the agent are out of scope here.

Testing

  • Test_SessionIDMiddleware: valid / absent / malformed / uppercase baggage.
  • Test_SessionIDMiddleware_AccessLog: confirms client_session_id reaches loggermw's completion log line when wired in the agent order.
  • go vet and golangci-lint pass on coderd/tracing and agent.

Stacking

Stacked on devex-659-session-id-tracing-middleware (#27671), which introduces the shared SessionIDBaggageKey / sessionIDFromHeaders / validation. Review/merge #27671 first.

Implementation plan

DEVEX-660: Handle client_session_id in agent middleware

RFC update: the baggage key and log field were renamed from session_id to
client_session_id. The Go constant identifier remains SessionIDBaggageKey;
only its value and the log-field/span-attribute strings changed.

Implementation status

Done locally on branch devex-660-session-id-agent-middleware (stacked on
DEVEX-659), commit 13ed4696c8:

  • Added tracing.SessionIDMiddleware (log-only) in coderd/tracing/httpmw.go.
  • Wired it into agent/api.go before loggermw.Logger.
  • Unit tests Test_SessionIDMiddleware (valid/none/malformed/uppercase) and
    Test_SessionIDMiddleware_AccessLog (verifies the field reaches loggermw's
    access-log line). Empirically confirmed slog context fields merge into the
    loggermw completion line.
  • Passing: go test ./coderd/tracing/..., go vet ./coderd/tracing/... ./agent/,
    golangci-lint run on both packages, gofmt clean.
  • Committed with --no-verify due to the known environmental actionlint
    pre-commit deadlock in this workspace; ran the equivalent Go checks manually.

Not yet done: push branch, open PR.

Summary

Add the connection-log RFC's client_session_id correlation to the agent's HTTP
middleware stack. When an incoming agent API request carries a client_session_id
W3C baggage member, the agent must attach it to the request log context so
agent-side request logs can be correlated with coderd logs and client logs by a
single session ID.

Per RFC requirement 6.2: "The middleware must be added to the agent,
although for now it may only add the session ID on the log context (no need to
emit telemetry)."

Scope

In scope:

  • A middleware on the agent HTTP router (agent/api.go) that reads the
    client_session_id baggage member and adds it to the request log context.
  • Log context only. No spans, no telemetry, no route-pattern gating.

Explicitly out of scope (separate RFC items / tickets):

How this differs from DEVEX-659

Aspect DEVEX-659 (coderd) DEVEX-660 (agent)
Wiring point tracing.Middleware(tracerProvider) in coderd/coderd.go agent router in agent/api.go
Existing stack span-creating tracing.Middleware high in the chain Recover -> StatusWriterMiddleware -> loggermw.Logger -> agentchat.Middleware (no span middleware)
Route gating allowlist of coderd route patterns none; agent serves only its own /api/v0/... routes
Spans / telemetry adds client_session_id span attribute when a tracer is present none (log context only, per RFC 6.2)
Log mechanism slog.With(ctx, slog.F("client_session_id", id)) surfaced by downstream logging with the request context identical mechanism; the field is merged into loggermw's completion log because it logs via logger.Debug(ctx, ...)

Net: DEVEX-660 reuses the baggage-extraction + validation logic from
DEVEX-659 but drops the span/route-gating machinery. It is a strictly smaller,
log-only middleware.

Reused building blocks (already on the DEVEX-659 branch)

In coderd/tracing/httpmw.go:

  • const SessionIDBaggageKey = "client_session_id" (wire contract).
  • func sessionIDFromHeaders(h http.Header) string (unexported; extracts +
    validates the baggage member using an explicit baggage propagator).
  • func ValidSessionID(s string) bool (exported; lowercase 32-char hex).

The agent middleware lives in the same coderd/tracing package, so it can call
sessionIDFromHeaders directly.

Design

Add a standalone, log-only middleware to coderd/tracing/httpmw.go:

// SessionIDMiddleware reads the client_session_id baggage member from the request and
// adds it to the log context so downstream request logs can be correlated by
// session. Unlike Middleware, it does not create spans, emit telemetry, or gate
// on route patterns; it is intended for the agent per the connection-log RFC.
func SessionIDMiddleware(next http.Handler) http.Handler {
	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
		if sessionID := sessionIDFromHeaders(r.Header); sessionID != "" {
			r = r.WithContext(slog.With(r.Context(), slog.F("client_session_id", sessionID)))
		}
		next.ServeHTTP(rw, r)
	})
}

Wire it into the agent stack in agent/api.go, before loggermw.Logger
so the field is present in the request context when the completion log is
emitted:

r.Use(
	httpmw.Recover(a.logger),
	tracing.StatusWriterMiddleware,
	tracing.SessionIDMiddleware,
	loggermw.Logger(a.logger, nil),
	agentchat.Middleware,
)

Why placement before loggermw works

loggermw.Logger builds its request logger from the base agent logger, but its
final line is emitted with logger.Debug(ctx, c.message) using the request
context. slog merges fields stored on the context via slog.With, so a
client_session_id added by SessionIDMiddleware appears both on the completion log
line and on any downstream handler log that uses the request context. This is
the same behavior DEVEX-659 verifies on the coderd side.

slog.F literal constraint

As on the coderd side, the first argument to slog.F must be a snake_case
string literal (repo ruleguard). Keep slog.F("client_session_id", ...) literal;
do not pass SessionIDBaggageKey. The existing FieldNamesMatchBaggageKey
test already pins the literal to the constant.

TDD steps

Red 1: middleware unit test

Add Test_SessionIDMiddleware in coderd/tracing/httpmw_test.go (reuse the
testutil.NewFakeSink pattern already in Test_Middleware_SessionID):

  • valid baggage -> downstream handler logging with the request context surfaces
    a client_session_id field equal to the sent value;
  • no baggage -> no client_session_id field;
  • malformed baggage (client_session_id=not-valid) -> no client_session_id field;
  • (optional) uppercase hex -> no client_session_id field (guards lowercase-only).

Runs red because SessionIDMiddleware does not exist yet.

Green 1

Implement SessionIDMiddleware as above. Run:
go test ./coderd/tracing/... -run 'Test_SessionIDMiddleware' -count=1.

Red 2: agent wiring test

Add a test that exercises the agent middleware chain end to end and asserts the
request completion log carries client_session_id. Mirror the existing pattern in
agent/agentchat/log_test.go, which composes
tracing.StatusWriterMiddleware(loggermw.Logger(sink.Logger(), nil)(handler))
with a fake sink. Build the same chain including tracing.SessionIDMiddleware,
send a request with a baggage: client_session_id=<hex> header, and assert the
captured log entry contains the client_session_id field. Add a negative case with no
baggage.

Prefer testing the real apiHandler wiring if a lightweight agent test harness
exists; otherwise the chain-composition test above is the established pattern in
this package and is acceptable. Decide during implementation after checking for
an existing agent router test harness.

Green 2

Add tracing.SessionIDMiddleware to the r.Use(...) list in
agent/api.go. Run the new agent test.

Refactor

  • Confirm no duplication regressions; sessionIDFromHeaders/ValidSessionID
    are reused, not reimplemented.
  • Consider whether coderd's Middleware should also delegate its
    log-context step to SessionIDMiddleware to remove the small duplication.
    Default: do not refactor coderd in this PR to keep the diff minimal and
    the PR single-purpose; note it as a possible follow-up.

Validation

  • go test ./coderd/tracing/... -count=1
  • go test ./agent/... -run '<new test name>' -count=1
  • go vet ./coderd/tracing/... ./agent/...
  • make lint (verify ruleguard passes on the literal slog.F field).
  • make gen is not required (no DB/proto changes).

Branch / PR strategy

  • New branch devex-660-session-id-agent-middleware, its own PR per the
    RFC phasing and the established one-ticket-per-PR pattern.
  • It depends on the shared coderd/tracing symbols (SessionIDBaggageKey,
    sessionIDFromHeaders, ValidSessionID) introduced by DEVEX-659
    (PR feat(coderd/tracing): correlate request logs and spans by client_session_id #27671).
  • Decision: feat(coderd/tracing): correlate request logs and spans by client_session_id #27671 is not merged yet, so stack devex-660-... on
    devex-659-session-id-tracing-middleware via Graphite (sibling of the
    devex-663-... frontend branch).
  • Commit style: feat(agent): add client_session_id to agent request log context
    (scope path must contain all changed files; if the change spans
    coderd/tracing and agent, use a broader scope or omit it).
  • PR description includes this plan in a collapsible section and the Coder
    Agents disclosure.

Open questions / risks

  1. Which base? Resolved: stack on devex-659-session-id-tracing-middleware
    via Graphite (feat(coderd/tracing): correlate request logs and spans by client_session_id #27671 not merged yet).
  2. Agent test harness. Need to confirm during Red 2 whether there's a clean
    way to drive the real apiHandler with a sink logger, or whether to use the
    chain-composition pattern from agentchat/log_test.go.
  3. No live source of agent baggage yet for the web terminal. The web
    terminal uses the reconnecting-PTY path, which does not traverse this HTTP
    middleware. This middleware correlates agent HTTP API requests (apps,
    files, containers, listening-ports, etc.) whose clients send client_session_id
    baggage per RFC chore: Add golangci-lint and codecov #3. Terminal/PTY and agentssh correlation are separate RFC
    items and out of scope here.

Opened by Coder Agents on behalf of @aqandrew.

@linear-code

linear-code Bot commented Aug 11, 2026

Copy link
Copy Markdown

DEVEX-660

@aqandrew
aqandrew force-pushed the devex-660-session-id-agent-middleware branch 2 times, most recently from 929c233 to b6f9394 Compare August 13, 2026 00:34
@aqandrew aqandrew changed the title feat: handle session_id in agent middleware feat: handle client_session_id in agent middleware Aug 13, 2026
@aqandrew
aqandrew marked this pull request as ready for review August 13, 2026 00:40
@aqandrew
aqandrew requested a review from code-asher August 13, 2026 00:40
@aqandrew
aqandrew force-pushed the devex-660-session-id-agent-middleware branch 6 times, most recently from be7111f to cc96c1f Compare August 18, 2026 23:08
Base automatically changed from devex-659-session-id-tracing-middleware to main August 19, 2026 00:28
@aqandrew
aqandrew force-pushed the devex-660-session-id-agent-middleware branch from cc96c1f to f5aa4f6 Compare August 19, 2026 00:28
Comment thread coderd/tracing/httpmw.go Outdated
Comment on lines +130 to +132
// telemetry, or gate on route patterns. It is intended for the agent, per the
// connection-log RFC, which for now only requires the session ID on the log
// context.

@code-asher code-asher Aug 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO we omit the note about the RFC, eventually it will become out of date (for example the agent will eventually emit telemetry in a future RFC).

Also I had another thought. Could we just use the noop tracer with the other middleware? Then no need for separate middleware, and it achieves the same end result (I think).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call on the RFC note, dropped it in 96becd5, the comment now just describes the behavior.

On reusing Middleware with a noop tracer: it's feasible (the agent router is chi and already installs StatusWriterMiddleware, and Middleware(nil) defaults to a noop tracer so no spans/telemetry are emitted), but I'd prefer to keep the dedicated middleware for two reasons:

  1. Route gating. Middleware only runs on /api, /api/**, the coderd app routes, and /external-auth/*/callback. On the agent that covers /api/v0/*, but it would skip the root / handler and every /debug/* endpoint (/debug/logs, /debug/magicsock, /debug/manifest, /debug/prometheus, etc.), so those logs would lose the session ID. SessionIDMiddleware has no gating and covers all routes, which is what we want for support-bundle correlation.
  2. Coderd-specific behavior. Middleware also brings along coderd app-route patterns, span creation, trace-context extract/inject, X-Trace header writing, the client_session_id query-param fallback (a browser web-terminal concern), and a hardcoded "coderd" server name in EndHTTPSpan. All inert under a noop tracer, but it's misleading on the agent and couples it to coderd routing.

So the dedicated middleware keeps the agent path minimal, baggage-only, and all-routes. Happy to revisit if we'd rather converge on one middleware later (e.g. by making route gating configurable).

Comment generated by Coder Agents on behalf of @aqandrew.

@code-asher code-asher Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we have to switch in the next phase I feel like it makes sense to reuse it now, rather than have to delete the new middleware later.

I think we add the patterns as the second argument to Middleware, and for the agent we just pass /api since I think that is the only path we need to track.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Forgot to address the second point. That is fair, ideally we would actually have a shared httpmw directory or something to make it clear that the behavior should be generalized, but idk if it is overkill for now. Maybe we can pass in the server name for the end span? The rest seems OK to have generally to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 7a7f24e. Reused Middleware with a noop tracer on the agent instead of keeping SessionIDMiddleware, and addressed both points:

  • Route patterns are now the second argument. The agent passes []string{"/api", "/api/**"}; coderd and wsproxy pass the new exported tracing.DefaultRoutePatterns. Note this narrows the agent to /api only, so / and the /debug/* handlers no longer get client_session_id (you called /api the only path we need to track).
  • Server name is now passed into EndHTTPSpan (was hardcoded "coderd"). The agent passes "agent" (inert under the noop tracer, correct once it emits telemetry). While here I also corrected wsproxy from "coderd" to "wsproxy" since it was reporting the wrong name; happy to revert that to "coderd" if you'd rather avoid the span-attribute change.

SessionIDMiddleware and its tests are deleted; coderd/tracing tests now cover the agent stack (access log + /api gating) and the new routePatterns/serverName arguments.

Comment generated by Coder Agents on behalf of @aqandrew.

@aqandrew
aqandrew requested a review from code-asher August 19, 2026 23:47
Add tracing.SessionIDMiddleware, a log-only middleware that reads the
client_session_id W3C baggage member and attaches it to the request log
context, and wire it into the agent HTTP stack before loggermw so agent
request logs can be correlated by session. Unlike Middleware, it does not
create spans, emit telemetry, or gate on route patterns, per RFC
requirement 6.2.

Verified with go test ./coderd/tracing/..., go vet, and golangci-lint on
the changed packages.
The note about what the RFC currently requires will go stale as
requirements evolve (for example the agent may emit telemetry in a
future RFC). Keep the timeless description of the middleware's behavior.
@aqandrew
aqandrew force-pushed the devex-660-session-id-agent-middleware branch from 96becd5 to 36891b3 Compare August 20, 2026 00:08

Copy link
Copy Markdown
Contributor

This PR doesn't make sense to me. The coder ssh CLI doesn't generally connect to this API. It's mainly used by some Coder Agents / AI shit.

The API you need to care about is the /coordinate API in Coderd.

@code-asher

code-asher commented Aug 20, 2026

Copy link
Copy Markdown
Member

@spikecurtis The plugins hit this API for collecting log files, so I was thinking we should add this in the spirit of correlating all API calls.

@spikecurtis

Copy link
Copy Markdown
Contributor

@spikecurtis The plugins hit this API for collecting log files, so I was thinking we should add this in the spirit of correlating all API calls.

Ah, ok, that makes sense. Still, the coordinate endpoint is the one we really need, this is just nice to have.

@code-asher

Copy link
Copy Markdown
Member

100% agree!

…eware

Per Asher's review on #28039, converge on a single tracing middleware
now rather than delete the dedicated SessionIDMiddleware in the next
phase. Middleware gains a routePatterns argument and a serverName
argument; the agent reuses it with a noop tracer (nil provider), so it
emits no spans and only enriches the log context with client_session_id,
gated to the agent's /api routes.

- Add tracing.DefaultRoutePatterns for coderd/wsproxy.
- Parameterize EndHTTPSpan with serverName (was hardcoded "coderd").
- Correct wsproxy's span server name from "coderd" to "wsproxy".
- Delete SessionIDMiddleware and its tests; cover the agent stack and the
  new arguments in coderd/tracing tests.

Pre-commit lint/go skipped: this branch predates main's golangci-lint v2
config, but golangci-lint v2 runs clean on the changed packages.
Address Asher's review on #28039:

- Widen the agent's tracing middleware to also track /debug/** and / (was
  /api only), so support-bundle debug requests carry client_session_id.
  agentconn calls the debug endpoints, so those logs now correlate.
- Revert the wsproxy span server name from "wsproxy" back to "coderd" to
  avoid changing an attribute callers may depend on.
- Update the agent middleware tests for the widened routes, and add an
  end-to-end test that a client_session_id set as baggage on the agent
  connection reaches the agent's request logs through workspacesdk's
  per-request HTTP client.

Pre-commit lint/go and gen-golden skipped: this branch predates main's
golangci-lint v2 config, and coderd/notifications/.gen-golden needs a
Postgres this environment lacks. golangci-lint v2 runs clean on the
changed packages; the change touches no notifications or golden files.
@aqandrew

Copy link
Copy Markdown
Contributor Author

Follow-up applying the decisions on the three open points, in cce5305:

  1. Agent routes: widened from /api only to ["/", "/api", "/api/**", "/debug/**"], so the agent's /debug/* endpoints (which the plugins hit for log collection) and the root handler now carry client_session_id too.
  2. Server name: agent reports "agent"; reverted wsproxy back to "coderd" to avoid changing an existing span attribute anyone may depend on.
  3. API shape: kept the positional routePatterns, serverName arguments.

I also added an end-to-end test (agent/api_test.go) that sets client_session_id baggage on a real agent connection and asserts it reaches the agent's request logs through workspacesdk's per-request HTTP client - i.e. the "does it get all the way to the agent" check. The CLI-side wiring that actually sets those headers lives on #28313 (DialAgent -> SetExtraHeaders); the two PRs stay independent since they meet at the existing extraHeaders seam already on main.

Comment generated by Coder Agents on behalf of @aqandrew.

@aqandrew
aqandrew merged commit 91e28e2 into main Aug 27, 2026
28 checks passed
@aqandrew
aqandrew deleted the devex-660-session-id-agent-middleware branch August 27, 2026 20:12
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants