diff --git a/CLAUDE.md b/CLAUDE.md index 226beb1f3..7a76242df 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -509,7 +509,7 @@ Envoy config lives in the `envoy-config` ConfigMap rendered by the [rossoctl Hel | 15123 | Envoy | TCP | Outbound listener (iptables redirects app traffic here) | | 15124 | Envoy | TCP | Inbound listener (iptables redirects incoming traffic here) | | 9090 | authbridge | gRPC | Ext-proc server (called by Envoy) | -| 9093 | authbridge | HTTP | Stats + config inspection (`/stats`, `/config`, `/reload/status`, `/tls-bridge/unread`) | +| 9093 | authbridge | HTTP | Stats + config inspection (`/stats`, `/config`, `/reload/status`, `/tls-bridge/unread`, and `/debug/pprof/*` where `listener.bind_loopback_only` is set) | | 9094 | authbridge | HTTP | Session events API (JSON snapshots + SSE stream) | | 9901 | Envoy | HTTP | Admin interface (bound to 127.0.0.1) | @@ -1209,7 +1209,7 @@ resulting `/shared/client-id.txt` and `/shared/client-secret.txt`. The schemas were the last big duplicate and needed a type change to reach. `InferenceTool.Parameters` was `map[string]any`, which cost **4.1x its JSON text** to hold and could not be interned without a recursive walk that rewrites map values — a walk cannot lean on string immutability the way sharing a string can. Measured on a live session: 84KB per event, ~172MB across one 2050-event session. So the field became `pipeline.RawJSON`, a named string type that keeps the schema exactly as the client sent it, and it interns like any other string. Two consequences worth knowing: the API now shows schemas in the client's own key order (a map round-trip silently sorted them), and the type must stay a NAMED string with a `MarshalJSON` method — OPA's `ast.InterfaceToValue` treats a plain or aliased string as a JSON string, which would leave every policy indexing `input.inference.tools[_].parameters` undefined with no error (`core/plugins/opa/tool_parameters_rego_test.go` guards it). MCP `Params`/`Result` still do not intern, being `map[string]any`; they are unmeasured on this workload and the same field-type change is available if that changes. - **Retention was only half of it, and reading the store was the more expensive half.** A memory figure that keeps climbing while traffic is idle is usually not retention: `GET /v1/sessions/{id}` used to encode the whole response into a single buffer before writing a byte, so serving one snapshot cost heap proportional to the *response* — ~246MB of heap growth for a 105MB response — and Go keeps that as idle heap rather than returning it promptly. agentop requests a snapshot on every Enter into a session, so RSS ratcheted a couple hundred megabytes per keystroke: four requests took a live proxy from 731MB to 1447MB, still 1447MB two minutes later. Interning could not have helped there, and the two are not alternatives — interning makes stored events *share* one copy of a repeated message, and an encoder expands every share back into its own bytes on the way out. The response is now written one event at a time (`core/sessionapi/snapshot.go`, `BenchmarkSnapshotHeap`). When judging any memory change here, note that **RSS is not heap** and the proxy exposes no heap metric — there is no `pprof` endpoint anywhere in this tree — so an RSS-per-event figure mixes retention with read-path churn and cannot settle either one. **`memory.limit_mb` (default 2048 on a local install, #1541) is the soft cap on exactly that gap**: `debug.SetMemoryLimit`, applied once at boot and refused on reload. Soft by the runtime's contract — near it the GC trades CPU for memory, and a live heap that genuinely exceeds it still grows rather than failing anything; the failure mode of too low a limit is slowness, never a crash. It counts Go-managed memory only, so the CPEX build's cgo allocations sit outside it. + **Retention was only half of it, and reading the store was the more expensive half.** A memory figure that keeps climbing while traffic is idle is usually not retention: `GET /v1/sessions/{id}` used to encode the whole response into a single buffer before writing a byte, so serving one snapshot cost heap proportional to the *response* — ~246MB of heap growth for a 105MB response — and Go keeps that as idle heap rather than returning it promptly. agentop requests a snapshot on every Enter into a session, so RSS ratcheted a couple hundred megabytes per keystroke: four requests took a live proxy from 731MB to 1447MB, still 1447MB two minutes later. Interning could not have helped there, and the two are not alternatives — interning makes stored events *share* one copy of a repeated message, and an encoder expands every share back into its own bytes on the way out. The response is now written one event at a time (`core/sessionapi/snapshot.go`, `BenchmarkSnapshotHeap`). When judging any memory change here, note that **RSS is not heap**: the stats server serves `/debug/pprof/*` only where `listener.bind_loopback_only` is set (the profiles expose stack traces and argv and let a caller start CPU/trace profiling), so on a cluster deployment an RSS-per-event figure mixes retention with read-path churn and nothing on the box can settle either one. **`memory.limit_mb` (default 2048 on a local install, #1541) is the soft cap on exactly that gap**: `debug.SetMemoryLimit`, applied once at boot and refused on reload. Soft by the runtime's contract — near it the GC trades CPU for memory, and a live heap that genuinely exceeds it still grows rather than failing anything; the failure mode of too low a limit is slowness, never a crash. It counts Go-managed memory only, so the CPEX build's cgo allocations sit outside it. **And the client is the other end of the same problem.** Measured on a laptop, `agentop` sat at 1.64GB against the proxy's 1.03GB — larger than the process it was watching, and the only one of the two still climbing. It made both mistakes the server had just stopped making: it decoded each snapshot as one document (`json.Decoder` only bounds its buffer between *values*, and a whole snapshot is one value), and it kept every string the server's encoder had expanded back out of the store's sharing. Both are fixed in `cmd/agentop/apiclient/snapshot.go`: the response is decoded one event at a time and fed through the same `session.Interner` the store uses, exported for the purpose rather than reimplemented. On a 23.5MB document, `+32.0MB HeapSys` and 26.49MB retained became +0.0MB and 2.31MB (`BenchmarkDecodeSessionViewHeap`). When a memory figure here looks wrong, measure **both** processes — the proxy has not been the larger one for a while. **The stream was the client's remaining leak (#1542):** `handleStreamEvent` appended every `/v1/events` arrival to `m.events[sessionID]` for EVERY session — never interned, never released — so an idle agentop grew at the wire rate (~18 MB/min; 1.5GB after 13h). Streamed events are now retained only for the session being watched (`watchingSession`: `selectedSess` — pane-agnostic, so pane walks don't punch a gap in the live tail — or the timeline's session); everything else updates its summary and is dropped, and opening a session fetches a fresh interned snapshot. The pipeline pane's per-plugin counts are folded into `model.pluginRuns` at stream arrival for every session — watched or not — so they still describe all traffic since attach (a snapshot's history contributes nothing, and a pod switch resets them). diff --git a/cmd/cortex/main.go b/cmd/cortex/main.go index 2e613ada4..7ba024d89 100644 --- a/cmd/cortex/main.go +++ b/cmd/cortex/main.go @@ -1143,6 +1143,10 @@ func main() { if bridge != nil { statOpts = append(statOpts, observe.WithTLSBridgeUnread(bridge.UnreadHandler())) } + // pprof exposes the process's internals (allocation stack traces, argv, + // caller-started CPU/trace profiling), so it rides the same loopback-only + // gate as the session API's clear. + statOpts = append(statOpts, observe.WithPprof(cfg.Listener.BindLoopbackOnly)) statSrv := bootstrap.ServeStatServer(rld.ConfigProvider(), statsProvider, rld.Handler(), pricingRegistry.Handler(), listeners.take("stats"), statOpts...) diff --git a/core/observe/statserver.go b/core/observe/statserver.go index 985a75d75..5962d959a 100644 --- a/core/observe/statserver.go +++ b/core/observe/statserver.go @@ -7,6 +7,8 @@ import ( "log/slog" "net" "net/http" + "net/http/pprof" + "strings" "time" "github.com/rossoctl/cortex/core/auth" @@ -38,6 +40,7 @@ type statServerOpts struct { reloadStatus http.Handler pricingTable http.Handler tlsBridgeUnread http.Handler + pprof bool } // WithReloadStatus registers a /reload/status handler (typically the @@ -64,6 +67,22 @@ func WithTLSBridgeUnread(h http.Handler) Option { return func(o *statServerOpts) { o.tlsBridgeUnread = h } } +// WithPprof mounts Go's net/http/pprof handlers under /debug/pprof/. Off unless +// asked for: the profiles expose the process's internals — allocation stack +// traces, argv (cmdline), and a caller's ability to start CPU profiling and +// tracing — so the host enables it only where the stats port is loopback-only +// (cmd/cortex reads listener.bind_loopback_only, the same gate as the session +// API's clear). The handlers also refuse a non-loopback Host and any request +// carrying Origin, the erase guards' browser defenses. +// +// Side effect of the import: net/http/pprof's init registers /debug/pprof/* on +// http.DefaultServeMux in every binary linking this package. Nothing in this +// tree serves that mux — an http.Server built with a nil Handler would, and +// would expose these ungated, so none may be. +func WithPprof(enabled bool) Option { + return func(o *statServerOpts) { o.pprof = enabled } +} + // NewStatServer builds the stat HTTP server. configProvider is // invoked per /config request and statsProvider per /stats request, // so both reflect current state rather than a snapshot captured at @@ -87,22 +106,37 @@ func NewStatServer(addr string, configProvider ConfigProvider, statsProvider Sta if o.tlsBridgeUnread != nil { mux.Handle("/tls-bridge/unread", o.tlsBridgeUnread) } + if o.pprof { + pprofGuard := func(h http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if !loopbackHost(r.Host) { + http.Error(w, "pprof is served only for a loopback Host", http.StatusForbidden) + return + } + if r.Header.Get("Origin") != "" { + http.Error(w, "pprof is refused from a browser: the request carries an Origin header", http.StatusForbidden) + return + } + h(w, r) + } + } + mux.HandleFunc("/debug/pprof/", pprofGuard(pprof.Index)) + mux.HandleFunc("/debug/pprof/cmdline", pprofGuard(pprof.Cmdline)) + mux.HandleFunc("/debug/pprof/profile", pprofGuard(pprof.Profile)) + mux.HandleFunc("/debug/pprof/symbol", pprofGuard(pprof.Symbol)) + mux.HandleFunc("/debug/pprof/trace", pprofGuard(pprof.Trace)) + } + indexPage := statServerIndex + if o.pprof { + indexPage = strings.Replace(statServerIndex, "{{pprof}}", + `
  • Go pprof profiles
    • `, 1) + } else { + indexPage = strings.Replace(statServerIndex, "{{pprof}}", "", 1) + } mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprint(w, ` - - - - - -`) + fmt.Fprint(w, indexPage) }) return &StatServer{ @@ -165,3 +199,36 @@ func (s *StatServer) Shutdown(ctx context.Context) error { func (s *StatServer) Close() error { return s.server.Close() } + +// statServerIndex is the / body, with {{pprof}} a slot WithPprof fills. +const statServerIndex = ` + + + + +` + +// loopbackHost reports whether hostport names exactly localhost, 127.0.0.1 or +// ::1, with or without a port. A twin of sessionapi's unexported loopbackHost +// (the erase guards): copied rather than shared because observe must not import +// the session API. A short allowlist, not 127.0.0.0/8 or a localhost suffix — +// the point is to refuse a name an attacker controls. +func loopbackHost(hostport string) bool { + host := hostport + if h, _, err := net.SplitHostPort(hostport); err == nil { + host = h + } + host = strings.TrimSuffix(strings.TrimPrefix(host, "["), "]") + switch strings.ToLower(host) { + case "localhost", "127.0.0.1", "::1": + return true + } + return false +} diff --git a/core/observe/statserver_test.go b/core/observe/statserver_test.go index 06087318c..a83115af7 100644 --- a/core/observe/statserver_test.go +++ b/core/observe/statserver_test.go @@ -196,3 +196,94 @@ func TestStatServer_TLSBridgeUnread(t *testing.T) { t.Error("a server built without the option served the report") } } + +func serveMuxOpts(cfg *config.Config, stats *auth.Stats, opts ...Option) http.Handler { + s := NewStatServer(":0", func() *config.Config { return cfg }, func() *auth.Stats { return stats }, opts...) + return s.server.Handler +} + +// WithPprof mounts Go's net/http/pprof handlers. The profiles expose the +// process's internals, so it must be possible to build the server without +// them, which is the default. +func TestStatServerPprofOffByDefault(t *testing.T) { + handler := serveMux(newTestConfig(), auth.NewStats()) + req := httptest.NewRequest(http.MethodGet, "/debug/pprof/heap", nil) + w := httptest.NewRecorder() + + handler.ServeHTTP(w, req) + + resp := w.Result() + body, _ := io.ReadAll(resp.Body) + if ct := resp.Header.Get("Content-Type"); !strings.HasPrefix(ct, "text/html") { + t.Errorf("GET /debug/pprof/heap without WithPprof served %q, want the HTML index", ct) + } + if strings.Contains(string(body), "/debug/pprof/") { + t.Error("index page links /debug/pprof/ without WithPprof") + } +} + +func TestStatServerPprofServesProfiles(t *testing.T) { + handler := serveMuxOpts(newTestConfig(), auth.NewStats(), WithPprof(true)) + req := httptest.NewRequest(http.MethodGet, "/debug/pprof/heap", nil) + req.Host = "localhost:47802" // a loopback Host, as a local client sends + w := httptest.NewRecorder() + + handler.ServeHTTP(w, req) + + resp := w.Result() + body, _ := io.ReadAll(resp.Body) + if resp.StatusCode != http.StatusOK { + t.Fatalf("GET /debug/pprof/heap status = %d, want 200", resp.StatusCode) + } + // A gzip stream is what pprof serves; the HTML index or an error page is not. + if len(body) < 2 || body[0] != 0x1f || body[1] != 0x8b { + t.Error("GET /debug/pprof/heap did not return a gzip profile stream") + } +} + +// A page can rebind a DNS name to 127.0.0.1 and have a browser read a loopback +// port, so the profiles refuse any Host that is not exactly loopback — the same +// defense as the session API's erase guards. +func TestStatServerPprofRefusesReboundHost(t *testing.T) { + handler := serveMuxOpts(newTestConfig(), auth.NewStats(), WithPprof(true)) + for _, host := range []string{"evil.example:47802", "127.0.0.1.evil.example", "[::1].evil.example"} { + req := httptest.NewRequest(http.MethodGet, "/debug/pprof/heap", nil) + req.Host = host + w := httptest.NewRecorder() + + handler.ServeHTTP(w, req) + + if w.Result().StatusCode != http.StatusForbidden { + t.Errorf("GET /debug/pprof/heap with Host %q = %d, want 403", host, w.Result().StatusCode) + } + } + // The loopback names themselves still pass. + for _, host := range []string{"localhost:47802", "127.0.0.1:47802", "[::1]:47802"} { + req := httptest.NewRequest(http.MethodGet, "/debug/pprof/heap", nil) + req.Host = host + w := httptest.NewRecorder() + + handler.ServeHTTP(w, req) + + if w.Result().StatusCode != http.StatusOK { + t.Errorf("GET /debug/pprof/heap with Host %q = %d, want 200", host, w.Result().StatusCode) + } + } +} + +// The Host check alone does not stop a browser: a simple no-cors GET from a +// page carries a loopback Host and no preflight. Browsers always send Origin, +// so its presence is the refusal — the erase guards' third check. +func TestStatServerPprofRefusesBrowserOrigin(t *testing.T) { + handler := serveMuxOpts(newTestConfig(), auth.NewStats(), WithPprof(true)) + req := httptest.NewRequest(http.MethodGet, "/debug/pprof/heap", nil) + req.Host = "127.0.0.1:47802" + req.Header.Set("Origin", "https://evil.example") + w := httptest.NewRecorder() + + handler.ServeHTTP(w, req) + + if w.Result().StatusCode != http.StatusForbidden { + t.Errorf("GET /debug/pprof/heap with Origin = %d, want 403", w.Result().StatusCode) + } +} diff --git a/docs/laptop-service.md b/docs/laptop-service.md index 364adc620..b58020d72 100644 --- a/docs/laptop-service.md +++ b/docs/laptop-service.md @@ -828,6 +828,21 @@ also keeps them as a list, which `agentop doctor` prints and the stats listener curl -s http://127.0.0.1:47602/tls-bridge/unread ``` +The stats port also serves Go's pprof profiles under `/debug/pprof/` — this answers +"what is the proxy holding in memory", which `ps` RSS cannot (freed pages the kernel +has not reclaimed still count as resident on macOS): + +```sh +curl -s --noproxy '*' http://127.0.0.1:47602/debug/pprof/heap -o /tmp/heap.pb.gz +go tool pprof -top -inuse_space /tmp/heap.pb.gz +``` + +`go tool pprof` cannot read the profile from a pipe, so the two-step form it is. The +`--noproxy '*'` matters on this machine: the shell's proxy env points at cortex itself. +The profiles expose stack traces and argv rather than object contents, but are served +only when `listener.bind_loopback_only` is true (the local install's default), and the +endpoints refuse a non-loopback Host or any browser Origin. + `programs` has one entry per program Cortex is not reading: `program` and `agent` (the executables), `reason`, `connections` (how many of the program's connections Cortex has passed through unread since Cortex started), `failures` and `stopped` (on `program-refused`),