diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 95e5a1e1..e3bb0623 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -17,8 +17,10 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-go@v5 with: - go-version: '1.25' - check-latest: true + # go.mod is the single source of truth for the toolchain. Pinning a literal here is + # what let CI validate on 1.25 while go.mod declared 1.26.4 and releases built on + # 1.26 (#152) — three numbers that have to agree and no mechanism making them. + go-version-file: go.mod - name: Lint run: make lint - name: Test & coverage @@ -54,8 +56,10 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-go@v5 with: - go-version: '1.25' - check-latest: true + # go.mod is the single source of truth for the toolchain. Pinning a literal here is + # what let CI validate on 1.25 while go.mod declared 1.26.4 and releases built on + # 1.26 (#152) — three numbers that have to agree and no mechanism making them. + go-version-file: go.mod - name: Build with no C compiler available run: | go build -o /tmp/cg-purego ./cmd/context-guru-proxy diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml new file mode 100644 index 00000000..bdc4786c --- /dev/null +++ b/.github/workflows/release.yaml @@ -0,0 +1,95 @@ +name: Release + +# Tag-driven, so a release is something a maintainer does on purpose. The `workflow_dispatch` +# entry builds the same matrix WITHOUT publishing (snapshot mode), which is how the release +# path gets exercised before there is a tag to regret. +on: + push: + tags: ["v*"] + workflow_dispatch: + +permissions: + contents: read + +jobs: + release: + runs-on: ubuntu-latest + permissions: + # Only the tag path publishes, and only this job needs the write. + contents: write + steps: + - uses: actions/checkout@v4 + with: + # GoReleaser's changelog needs the history the default shallow clone does not have. + fetch-depth: 0 + - uses: actions/setup-go@v5 + with: + # Same source as CI (go.mod), deliberately: an artifact people download must be built + # with the toolchain CI validated, and a literal here is how that drifts apart. + go-version-file: go.mod + + # The claim the release rests on, asserted in CI rather than trusted: the shipped + # binary needs no C toolchain. CGO_ENABLED=0 with no compiler on PATH would fail loudly + # here if a cgo dependency ever escaped the cg_skeleton build tag — which is exactly the + # regression that would otherwise be discovered by an evaluator, at install time. + - name: Assert the binary is pure Go + env: + CGO_ENABLED: "0" + CC: /nonexistent-c-compiler + run: | + go build -o /tmp/cg-purego ./cmd/context-guru-proxy + file /tmp/cg-purego | tee /dev/stderr | grep -q "statically linked" + # And it has to actually start, not just link. + /tmp/cg-purego --listen 127.0.0.1:4471 --preset cache & + for _ in $(seq 1 40); do + sleep 0.25 + curl -fsS http://127.0.0.1:4471/healthz && break + done + curl -fsS http://127.0.0.1:4471/healthz | grep -q ok + # An installer asks the binary what it is; make sure it can answer. + /tmp/cg-purego --version | tee /dev/stderr | grep -q context-guru-proxy + + # Nothing tested the configuration we actually SHIP. + # + # ci.yaml runs the suite only with CGO_ENABLED=1, and a tag push previously published + # without running any tests at all. So the one guard that matters most to a released + # artifact — TestEveryPresetBuilds, which catches a preset naming a component that is not + # registered in a CGO-free binary — was never executed in the CGO-free configuration. That + # is exactly the `preset: coding` / `unknown component "skeleton"` failure, in a build no + # developer runs locally. + # + # The race detector needs cgo, so this cannot be the whole suite; it is the packages whose + # behaviour depends on which components are compiled in. + - name: Test the shipped configuration (CGO off, no race detector) + env: + CGO_ENABLED: "0" + # -p 1 for the reason ci.yaml's purego job documents: on a 2-core runner, parallel package + # binaries provoked a real flake (#163), and this workflow runs the full suite a few steps + # below as well, so the contention here is at least as bad. A tag push must not fail to + # publish for a cause already diagnosed and mitigated one file over. + run: go test -p 1 ./config/... ./components/... ./apply/... ./proxy/... ./store/... + + # A tag must not publish something the full suite has not seen. + - name: Full test suite + env: + CGO_ENABLED: "1" + run: go test ./... + + - name: Release + uses: goreleaser/goreleaser-action@v6 + with: + version: "~> v2" + # A tag publishes; a manual run builds the full matrix and publishes nothing. + args: ${{ startsWith(github.ref, 'refs/tags/v') && 'release --clean' || 'release --clean --snapshot' }} + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Upload snapshot artifacts + if: ${{ !startsWith(github.ref, 'refs/tags/v') }} + uses: actions/upload-artifact@v4 + with: + name: snapshot-dist + path: | + dist/*.tar.gz + dist/checksums.txt + retention-days: 7 diff --git a/.goreleaser.yaml b/.goreleaser.yaml new file mode 100644 index 00000000..d628b182 --- /dev/null +++ b/.goreleaser.yaml @@ -0,0 +1,111 @@ +# GoReleaser: the release artifacts an evaluator downloads instead of installing a toolchain. +# +# The whole file is a plain GOOS/GOARCH matrix with no C cross-toolchains, no zig, and no +# libc coupling, because `CGO_ENABLED=0` builds the shipped binary. That is verified rather +# than assumed — the `Assert the binary is pure Go` step in .github/workflows/release.yaml fails +# the release if a cgo dependency ever escapes the cg_skeleton build tag. Measured directly on +# go 1.26.4: +# all four targets build, the artifact is 27–34 MB stripped, `file` reports "statically +# linked" and `ldd` "not a dynamic executable", and the resulting binary serves /healthz. +# +# `cg_skeleton` is the ONE thing that needs cgo (tree-sitter), and it is deliberately not +# built here: it is in no default preset, not in the cache story, and shipping it would mean +# per-platform C cross-compilation for a component this funnel never runs. Source build is +# documented in docs/components/skeleton.md. +# +# There is no `brews:` block yet — the tap repo and release signing are an open ownership +# question (spec §"Open questions", 3). Until it is answered the funnel installs from the +# release tarball, so nothing here depends on a repo that does not exist. Adding the tap +# later is additive and changes none of the below. +version: 2 + +project_name: context-guru + +before: + hooks: + - go mod download + +builds: + - id: context-guru-proxy + main: ./cmd/context-guru-proxy + binary: context-guru-proxy + env: + # The point of the whole file. Not inherited from the Makefile, which sets + # CGO_ENABLED=1 because `go test -race` needs it — a test-time requirement that was + # being read as a shipping requirement. + - CGO_ENABLED=0 + flags: + # Reproducible paths in panics, and no VCS stamping (the checkout is shallow in CI). + - -trimpath + - -buildvcs=false + ldflags: + # Same two symbols the Makefile stamps, so `/stats` build_version is populated in a + # released binary exactly as it is in a locally built one. + - -s -w + - -X github.com/rossoctl/context-guru/internal/buildinfo.Version={{ .Version }} + - -X github.com/rossoctl/context-guru/internal/buildinfo.Commit={{ .ShortCommit }} + goos: [linux, darwin] + goarch: [amd64, arm64] + +archives: + - id: default + ids: [context-guru-proxy] + # An evaluator untars this into ~/.local/bin, so the archive name is what they see and + # the binary inside must be the plain name with no version in it. + name_template: >- + {{ .ProjectName }}_{{ .Version }}_{{ .Os }}_{{ .Arch }} + formats: [tar.gz] + # Everything unpacks into ONE directory named after the archive. + # + # Without this GoReleaser writes a flat archive, so LICENSE, README.md and THIRD-PARTY-NOTICES + # land at the root — and the extraction command in the footer below (and in the quickstart) has + # no -C, so running it in a project directory silently overwrites that project's own README.md + # and LICENSE. A person evaluating a proxy for their coding agent is standing in exactly such a + # directory. + wrap_in_directory: true + files: + - LICENSE + - README.md + - THIRD-PARTY-NOTICES + +checksums: + # The integrity check for every downloaded artifact, and the ONLY one: these binaries are + # unsigned (the tap and signing ownership are still open), so nothing else stands between a + # tampered tarball and a proxy that handles the user's LLM traffic. + # + # The plugin's installer verifies against this file and strips macOS quarantine from the + # download, which is what makes it load-bearing rather than decorative. That installer is NOT in + # this change — it ships with the plugin — so today this file is what a human curling a release + # should check by hand. + name_template: checksums.txt + algorithm: sha256 + +snapshot: + version_template: "{{ incpatch .Version }}-next" + +changelog: + use: github + sort: asc + filters: + exclude: + - "^docs:" + - "^test:" + - "^chore:" + - "^ci:" + +release: + prerelease: auto + footer: | + ## Install + + No Go toolchain and no C compiler are needed — the binary is statically linked. + + Download the tarball for your platform, untar it, and put `context-guru-proxy` on your + `PATH`. The archive unpacks into its own directory, so this is safe to run anywhere: + + ``` + tar xzf context-guru_*_darwin_arm64.tar.gz + install -m 755 context-guru_*/context-guru-proxy ~/.local/bin/ + ``` + + Then see `docs/get-started/quickstart-proxy.md`. diff --git a/README.md b/README.md index 3fd159ec..582ae3a4 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,9 @@ docker build -t context-guru:local . ## Quickstart (60 seconds) +Download a release binary — statically linked, **no Go and no C compiler needed** — or build +from source: + ```sh # 1 — run the proxy (ships with the SWE-bench-winning cache-aware config by default) ./bin/context-guru-proxy # --preset house (the default); listens on :4000 @@ -144,8 +147,10 @@ See [docs/components.md](docs/components.md) and [docs/reference/presets.md](doc | Flag / env | Default | Purpose | |---|---|---| | `--preset` / `PRESET` | `house` | pipeline preset when no `--config` | +| `--idle-exit` / `IDLE_EXIT` | `0` (never) | exit after this long unused; floor `max(2 × store.ttl_seconds, 1h)`, refused with `--upstreams` | +| `--version` | — | print version and commit, then exit | | `--config` / `CONFIG` | — | YAML config (overrides preset) | -| `LISTEN_ADDR` | `:4000` | listen address | +| `--listen` / `LISTEN_ADDR` | `:4000` | listen address. The flag exists so the port is visible in `ps` and to a supervisor | | `--anthropic-upstream` / `ANTHROPIC_UPSTREAM` | `https://api.anthropic.com` | Anthropic upstream base | | `--openai-upstream` / `OPENAI_UPSTREAM` | `https://api.openai.com` | OpenAI upstream base | | `OPENAI_API_KEY` / `ANTHROPIC_API_KEY` | — | real key injected on forward (gateway mode); empty = pass client auth through | diff --git a/cmd/context-guru-proxy/idleexit.go b/cmd/context-guru-proxy/idleexit.go new file mode 100644 index 00000000..4ab3850d --- /dev/null +++ b/cmd/context-guru-proxy/idleexit.go @@ -0,0 +1,290 @@ +package main + +import ( + "fmt" + "net/http" + "strings" + "sync/atomic" + "time" + + "github.com/rossoctl/context-guru/store" +) + +// Idle-exit: a proxy that a Claude Code session started should not outlive the machine's use +// of it. +// +// The funnel installs a SessionStart hook that starts the proxy on demand, so nothing has to +// be left running — but only if the process eventually goes away on its own. That is all this +// is: a clock, a probe, and the SAME graceful shutdown path SIGTERM takes. No new teardown +// logic, because the teardown is the part that is already right (armShutdown releases the +// dashboard's SSE connections, the deferred closes flush the capture batch). +// +// It is OFF unless asked for. A gateway deployment or an eval-containers run must never +// self-terminate, and "the proxy vanished overnight" is a much worse failure there than a +// process left running on a laptop. +// +// Two things make this less trivial than a timeout, and both are load-bearing: +// +// 1. **The keep-alive inverts "idle".** Pinging is what the proxy does WHILE no client +// traffic arrives, so a watchdog that watches requests alone kills the feature in +// precisely its working window. Hence the pending probe below, which both blocks exit and +// resets the clock. +// 2. **Exit wipes the in-memory store.** A threshold shorter than the store's entry lifetime +// drops live frozen decisions and re-bills their prefix at cache-creation prices. That is +// refused at startup, not documented — see store.ValidateIdleExit. + +// activityClock is the last moment this process did something a user would call "in use". +// +// It stores the time.Time itself, not its Unix nanoseconds, and that is the whole point: a +// time.Time from time.Now() carries a MONOTONIC reading, and Sub between two such values uses it. +// Rebuilding the instant with time.Unix(0, ns) throws that reading away, leaving wall-clock +// arithmetic — so a laptop suspend/resume or an NTP step counts as idleness, and the watchdog can +// fire on its first tick after a lid-open, racing the user's first request. On the laptop this +// feature exists for, suspend is not an edge case. +// +// An atomic.Pointer costs one small allocation per request instead of one integer store. That is +// noise beside what net/http already allocates per request, and it buys a clock that measures +// elapsed time rather than calendar time. +type activityClock struct{ at atomic.Pointer[time.Time] } + +func (a *activityClock) touch(now time.Time) { a.at.Store(&now) } + +// last returns the stored instant, or the zero Time if nothing has been stamped yet. +func (a *activityClock) last() time.Time { + if p := a.at.Load(); p != nil { + return *p + } + return time.Time{} +} + +// probeRoutes are the request PATHS that do not count as use. +// +// They are what a machine asks, not what a person or an agent does: a Kubernetes liveness probe, a +// Prometheus scrape, a `curl /healthz` in a monitoring loop, the session hook's own start-up check. +// Counting them disabled the whole feature rather than weakening it — measured: a proxy with a 1h +// threshold logged `idle-exit armed after=1h0m0s` and then reported `idle for 1h3m0s` after 2h03m of +// wall clock, because a /healthz poller had been stamping the clock for the first hour. Any probe on +// a schedule shorter than the threshold means the exit NEVER fires, and nothing logs that it stopped +// working. +// +// Everything else still counts, including the dashboard's own polling: a person with the dashboard +// open is using this process, and exiting under them is a worse failure than a process left running. +// That asymmetry is deliberate — a probe is not a viewer. +// +// Matched against the MUX PATTERN rather than r.URL.Path; see stampActivity for why. +var probeRoutes = map[string]bool{ + "/healthz": true, + "/metrics": true, +} + +// patternPath strips the optional method from a ServeMux pattern: Go 1.22 patterns may be +// "GET /healthz", and this proxy registers them that way. +func patternPath(pattern string) string { + if i := strings.LastIndexByte(pattern, ' '); i >= 0 { + return pattern[i+1:] + } + return pattern +} + +// isCatchAll reports whether this pattern is the "/" route, which matches anything nothing else +// claimed. +// +// Bob mode and hosted mode both register one (proxy.Mux, `m.HandleFunc("/", h.passthrough(…))`), and +// with it present `mux.Handler` answers "/" — not the empty pattern — for `/healthz/`, `/nope` and +// every port-scan path. So the 404 branch above stops working and the activity clock is stamped +// forever: --idle-exit never fires, silently, exactly as it did before any of this. +// +// checkIdleExit now refuses --idle-exit alongside --bob-upstream as well as --upstreams, which +// closes the hole by construction — a proxy with a catch-all cannot also have a watchdog. This check +// is the belt to that braces: the two conditions live in different files, and if they ever drift +// apart, over-counting the catch-all as "not use" fails toward exiting a laptop proxy rather than +// toward a gateway that never exits. +func isCatchAll(pattern string) bool { return patternPath(pattern) == "/" } + +// stampActivity records a request as activity, unless its route is a machine probe. +// +// Stamped on entry AND on completion. The entry stamp is what makes a burst of short requests +// keep the process alive; the completion stamp is what stops a long request from being treated as +// a gap in use once it finishes. +// +// Be precise about what this does NOT fix, because an earlier version of this comment claimed the +// opposite: the clock is not refreshed DURING a request, so a single request that outlives the +// whole threshold with no other traffic can still age out mid-flight — the shape being a lone SSE +// consumer on /api/events, which armShutdown deliberately severs so srv.Shutdown can finish. +// Periodic stamping from inside a handler is the only thing that would close that, and it is not +// worth the machinery: the dashboard UI polls every 30s, so its SSE stream is never the only +// traffic in practice, and the threshold's floor is an hour. +func stampActivity(mux *http.ServeMux, act *activityClock, now func() time.Time) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Ask the mux which pattern this request resolves to, rather than comparing r.URL.Path. + // + // What an exact path compare got wrong: `/healthz/` was not exempt, so a monitoring loop or + // a Kubernetes httpGet probe configured with a trailing slash counted as use and the exit + // never fired. Asking the mux covers that case and every other spelling — `//healthz`, dot + // segments, a typo — with one question, using the matcher that will actually route the + // request rather than a second copy of the rules. + // + // Be precise about the MECHANISM, because an earlier version of this comment was not and the + // claim is what justifies the approach: ServeMux does NOT redirect `/healthz/` to `/healthz`. + // cleanPath re-appends a trailing slash, and matchOrRedirect only ever ADDS one (`/tree` -> + // `/tree/`), never strips it. With this route table `/healthz/` is a plain 404 and Handler + // reports the EMPTY pattern — which is what exempts it. The redirect Go does generate, for a + // subtree root, also reports an empty pattern, so nothing here may assume that a redirect + // resolves to its post-redirect pattern. + // + // So the rule is: a request counts as use only when it matched a REAL route that is not a + // probe. An unmatched path — a port scanner, a stray `/health`, a trailing slash — reports + // the empty pattern and does not count. + _, pattern := mux.Handler(r) + use := pattern != "" && !probeRoutes[patternPath(pattern)] && !isCatchAll(pattern) + if use { + act.touch(now()) + } + mux.ServeHTTP(w, r) + if use { + act.touch(now()) + } + }) +} + +// idleExitOptions is everything watchIdle needs, with the clock and the ticker injected so +// the policy is testable without waiting out a real threshold. +type idleExitOptions struct { + // threshold is how long the proxy must be unused before it exits. + threshold time.Duration + // act is stamped by stampActivity on every request. + act *activityClock + // pending reports work that must keep the process alive even with no requests: keep-alive + // sessions with a ping still ahead of them. nil means "nothing pending, ever". + pending func() int + now func() time.Time + // tick drives the check. Production uses a ticker at a fraction of the threshold; the + // resolution only bounds how late the exit is, never how early. + tick <-chan time.Time + // stop abandons the watch (the process is shutting down for another reason). + stop <-chan struct{} +} + +// watchIdle blocks until the proxy has been idle for the whole threshold, and returns a +// human-readable reason for the log. ok is false when the watch was abandoned via stop. +// +// Pending keep-alive work does not merely veto the exit, it RESETS the clock. Vetoing alone +// would exit the instant the last ping retired, taking the store with it at the moment a +// session is most likely to come back — the quiet gap after `end_turn` is where the pings +// were aimed in the first place. Treating a pending ping as activity gives the session a full +// threshold of grace after its last one. +func watchIdle(o idleExitOptions) (string, bool) { + if o.now == nil { + o.now = time.Now + } + // A BACKSTOP, not the real seed. The caller stamps the clock at launch (main), which is + // the only place that knows when "launch" was; seeding here would date the clock from + // whenever this goroutine happened to get scheduled, which is both later and unknowable. + // It stays because the failure mode of an unstamped clock is the worst one available — a + // zero clock reads as "idle since 1970" and exits on the first tick. + // IsZero, not UnixNano()==0: the clock now stores a time.Time to keep its monotonic reading, + // and time.Time{}.UnixNano() is a large NEGATIVE number, not zero. Testing the old way made + // this backstop stop firing, and an unstamped clock then read as "idle since the zero year" — + // the watchdog exited on its first tick, reporting "idle for 2562047h47m16s". Caught by + // TestIdleExitStartsItsClockAtLaunch, which exists for exactly this failure. + if o.act.last().IsZero() { + o.act.touch(o.now()) + } + for { + select { + case <-o.stop: + return "", false + case <-o.tick: + now := o.now() + if o.pending != nil { + if n := o.pending(); n > 0 { + o.act.touch(now) + continue + } + } + if idle := now.Sub(o.act.last()); idle >= o.threshold { + return "idle for " + idle.Round(time.Second).String() + + " (--idle-exit " + o.threshold.String() + ")", true + } + } + } +} + +// idleCheckInterval is how often the watchdog looks: a twentieth of the threshold, capped at five +// minutes so a 24h threshold does not mean an hour of slack past the moment it was asked for. +// +// There is no lower clamp, and there was one that could never fire. checkIdleExit refuses any +// threshold below an hour, so threshold/20 is at least three minutes for every value that reaches +// here — the old `if d < 30*time.Second` branch was unreachable in production, and the comment +// above it claimed the clamps stopped "a 1h floor meaning a check every three minutes" when three +// minutes is exactly what an hour yields. Removing it is the honest version: the resolution at the +// floor IS three minutes, which is 5% of the threshold, which is the rule. +func idleCheckInterval(threshold time.Duration) time.Duration { + d := threshold / 20 + // A panic guard, not policy. time.NewTicker panics on a non-positive duration, and integer + // division makes that reachable for any threshold under 20ns — which checkIdleExit refuses, so + // this cannot fire in production. It exists because the last version of this function reasoned + // "checkIdleExit refuses anything under an hour" and was then reached with 10ns through a path + // that skipped the floor entirely: a crash is the wrong failure mode for a helper, whatever the + // caller did. + if d <= 0 { + return time.Nanosecond + } + if d > 5*time.Minute { + return 5 * time.Minute + } + return d +} + +// checkIdleExit is every reason a requested idle-exit threshold must not start. +// +// A function rather than two inline `if`s in main so both refusals are testable: they are +// startup-fatal, which is the one class of check where "it looked right" is the only evidence +// anyone ever gathers. +func checkIdleExit(d time.Duration, upstreamsPath, bobUpstream string, o store.Options) error { + // The floor has two terms and only one of them is about the store. + // + // `2 x store.ttl_seconds` protects the in-memory store: exiting clears it, and losing a live + // frozen decision re-bills its whole prefix as cache creation (the 11.5x regression FrozenLost + // exists to catch). That term genuinely does not apply with `--store=false` / `STORE=false`, + // which resolves to store.Nop and holds no frozen decisions. + // + // The bare 1h term is not about the store at all, and dropping it was a mistake: skipping the + // whole check let `STORE=false --idle-exit=10ns` through, and threshold/20 then reached + // time.NewTicker as 0, which PANICS. An intended startup refusal became a crash. It also broke + // the invariant README and docs/reference/config.md state unconditionally. + // + // So: store off means the 1h minimum still applies, and the TTL term is what is skipped. + // `Enabled == nil` is "not configured", which is ON, so only an explicit false takes this path. + if o.Enabled != nil && !*o.Enabled { + if d > 0 && d < time.Hour { + return fmt.Errorf("idle-exit %s is below the 1h minimum. The store is disabled, so the "+ + "usual floor of 2x store.ttl_seconds does not apply — but a threshold this short is "+ + "still shorter than the keep-alive's own ping window, and a sub-second one cannot be "+ + "scheduled at all", d) + } + } else if err := store.ValidateIdleExit(d, o); err != nil { + return err + } + if d > 0 && (upstreamsPath != "" || bobUpstream != "") { + // A self-terminating GATEWAY is a different kind of wrong: --upstreams means this + // process serves other people's agents, where "the proxy vanished overnight" is far + // worse than a process left running on a laptop. + // + // The safety used to be accidental — it held only because a hosted deployment also runs + // a liveness probe, and every probe stamped the activity clock. That is no longer true + // (probeRoutes above deliberately excludes them), so what was accidentally safe is now + // explicitly refused rather than quietly reintroduced. + flag := "--upstreams" + if upstreamsPath == "" { + flag = "--bob-upstream" + } + return fmt.Errorf("--idle-exit cannot be combined with %s: a gateway serving other "+ + "people's agents must not self-terminate. Drop --idle-exit, or run this instance "+ + "without %s.\n\nBoth flags also mount a `/` catch-all route, which is the second "+ + "reason: with one registered, every unmatched path — including a probe with a trailing "+ + "slash — matches a real pattern and counts as activity, so the watchdog would never "+ + "fire and nothing would say so", flag, flag) + } + return nil +} diff --git a/cmd/context-guru-proxy/idleexit_test.go b/cmd/context-guru-proxy/idleexit_test.go new file mode 100644 index 00000000..4530b5b2 --- /dev/null +++ b/cmd/context-guru-proxy/idleexit_test.go @@ -0,0 +1,656 @@ +package main + +import ( + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/rossoctl/context-guru/store" +) + +// The shipped idle-exit default is 24h, so these tests drive a hand-advanced clock and a +// hand-fed ticker instead of waiting. watchIdle reads the time from o.now() and treats a tick +// purely as "look now", so the value carried on the channel is irrelevant and a tick that +// arrives late still evaluates against the current fake clock. + +type fakeClock struct{ ns atomic.Int64 } + +func newFakeClock(t time.Time) *fakeClock { + c := &fakeClock{} + c.ns.Store(t.UnixNano()) + return c +} +func (c *fakeClock) now() time.Time { return time.Unix(0, c.ns.Load()) } +func (c *fakeClock) advance(d time.Duration) { c.ns.Add(int64(d)) } + +// watcher drives one watchIdle and reports its verdict. +// +// Two things here are deliberate, and both were bugs first: +// +// - **The tick channel is UNBUFFERED.** With a buffer, the first send succeeds against the +// buffer whether or not the watcher goroutine has been scheduled at all — so a test could +// advance its clock believing the watcher had already started, and then measure idleness +// from the wrong instant. Unbuffered makes a send a rendezvous: it completes only once the +// watcher has actually received it. +// - **Every interaction selects on the result channel too.** The moment the watcher exits it +// stops draining ticks, and an unconditional send then blocks until the test deadline — +// a hang, which tells you nothing, rather than a failure. +type watcher struct { + t *testing.T + tick chan time.Time + res chan string + clk *fakeClock +} + +func start(t *testing.T, clk *fakeClock, o idleExitOptions) *watcher { + return startWith(t, clk, o, false) +} + +// startWith exposes the one knob start hides: seedAtLaunch=true leaves the activity +// clock unstamped, so watchIdle's own backstop is what gets tested. +func startWith(t *testing.T, clk *fakeClock, o idleExitOptions, seedAtLaunch bool) *watcher { + t.Helper() + w := &watcher{t: t, tick: make(chan time.Time), res: make(chan string, 1), clk: clk} + o.tick = w.tick + o.now = clk.now + // Stamp the clock the way main does at launch, unless the test is specifically exercising + // the unstamped case. + if !seedAtLaunch { + o.act.touch(clk.now()) + } + go func() { + reason, ok := watchIdle(o) + if !ok { + reason = "" // abandoned via stop + } + w.res <- reason + }() + // Synchronise before returning, with a real tick rather than a sleep: on an unbuffered + // channel a completed send proves the watcher is running and has reached its select, so a + // clock the test advances afterwards cannot be mistaken for the launch time. + // + // It doubles as an assertion: at zero elapsed time nothing may exit. + if verdict, done := w.poke(); done { + t.Fatalf("watchIdle exited (%q) on its first look, with no time elapsed", verdict) + } + return w +} + +// poke delivers one tick, or reports the verdict if the watcher has already finished. +func (w *watcher) poke() (verdict string, done bool) { + w.t.Helper() + select { + case r := <-w.res: + return r, true + case w.tick <- w.clk.now(): + return "", false + case <-time.After(3 * time.Second): + w.t.Fatal("watchIdle is neither consuming ticks nor returning") + return "", true + } +} + +// mustNotExit checks the watcher evaluated the current clock and stayed alive. +// +// It pokes TWICE on purpose: the tick channel holds one, so a second successful send proves +// the first was consumed and the loop came back for more, rather than merely sitting in the +// buffer unexamined. Without that, "no exit" could just mean "never looked". +func (w *watcher) mustNotExit(what string) { + w.t.Helper() + for i := 0; i < 2; i++ { + if verdict, done := w.poke(); done { + w.t.Fatalf("%s: watchIdle exited (%q) when it must not", what, verdict) + } + } +} + +// mustExit gives the watcher a bounded number of looks to decide it is idle. +func (w *watcher) mustExit(what string) string { + w.t.Helper() + for i := 0; i < 4; i++ { + if verdict, done := w.poke(); done { + if verdict == "" { + w.t.Fatalf("%s: the watch was abandoned instead of exiting", what) + } + return verdict + } + } + w.t.Fatalf("%s: idle past the threshold, but watchIdle never exited", what) + return "" +} + +// TestIdleExitFiresWhenNothingIsHappening is the base case the feature exists for: a proxy a +// session started, and then nobody used, goes away by itself instead of being left on the +// evaluator's machine. +func TestIdleExitFiresWhenNothingIsHappening(t *testing.T) { + clk := newFakeClock(time.Unix(1_700_000_000, 0)) + w := start(t, clk, idleExitOptions{threshold: time.Hour, act: &activityClock{}, + pending: func() int { return 0 }, stop: make(chan struct{})}) + + clk.advance(30 * time.Minute) + w.mustNotExit("half a threshold") + + clk.advance(31 * time.Minute) + t.Logf("exit reason: %s", w.mustExit("past the threshold")) +} + +// TestIdleExitWaitsForAPendingKeepAlivePing is the case a naive watchdog gets wrong. +// +// The keep-alive INVERTS the meaning of idle: pinging is what the proxy does precisely while +// no client traffic is arriving — the quiet gap after `end_turn`, where 83.7% of the +// recoverable dollars sit. A watchdog counting requests alone would kill the process in +// exactly the window the feature was built for. +// +// Two properties, the second subtler than the first: +// +// 1. a pending ping VETOES the exit, however long the client silence; +// 2. it also RESETS the clock, so retiring the last ping does not exit moments later — it +// buys a full fresh threshold. Veto-only would drop the in-memory store at the instant +// the session is most likely to come back, which is the cache-write regression the floor +// and this whole feature are meant to avoid. +func TestIdleExitWaitsForAPendingKeepAlivePing(t *testing.T) { + clk := newFakeClock(time.Unix(1_700_000_000, 0)) + var pending atomic.Int64 + pending.Store(1) + w := start(t, clk, idleExitOptions{threshold: time.Hour, act: &activityClock{}, + pending: func() int { return int(pending.Load()) }, stop: make(chan struct{})}) + + // (1) Veto: two full thresholds of silence with a ping still scheduled. + clk.advance(2 * time.Hour) + w.mustNotExit("a keep-alive ping is still scheduled") + + // (2) The ping retires. If the veto reset the clock, a threshold measured from the START + // is not enough — only 30m have passed since the last pending observation. + pending.Store(0) + clk.advance(30 * time.Minute) + w.mustNotExit("30m after the last ping retired") + + clk.advance(31 * time.Minute) + w.mustExit("genuinely idle for a whole threshold") +} + +// TestRequestsDeferIdleExit covers the stamping half: a real request is use, and use defers the +// exit. +func TestRequestsDeferIdleExit(t *testing.T) { + clk := newFakeClock(time.Unix(1_700_000_000, 0)) + act := &activityClock{} + stampedBeforeHandler := false + mux := http.NewServeMux() + mux.HandleFunc("POST /anthropic/v1/messages", func(w http.ResponseWriter, r *http.Request) { + // The stamp must land BEFORE the handler runs, so a burst of requests keeps the clock warm + // without waiting for each to finish. + stampedBeforeHandler = act.last().Equal(clk.now()) + w.WriteHeader(http.StatusOK) + }) + h := stampActivity(mux, act, clk.now) + + w := start(t, clk, idleExitOptions{threshold: time.Hour, act: act, + pending: func() int { return 0 }, stop: make(chan struct{})}) + + clk.advance(50 * time.Minute) + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("POST", "/anthropic/v1/messages", nil)) + if !stampedBeforeHandler { + t.Fatal("stampActivity did not record the request before invoking the handler") + } + // 80m since launch, but only 30m since the request. + clk.advance(30 * time.Minute) + w.mustNotExit("30m after serving a request") + + clk.advance(31 * time.Minute) + w.mustExit("an hour after the last request") +} + +// TestIdleExitStopAbandonsTheWatch: when the process is already shutting down for another +// reason (SIGTERM), the watchdog must let go rather than hold a goroutine and push a second +// reason into the shutdown path. +func TestIdleExitStopAbandonsTheWatch(t *testing.T) { + clk := newFakeClock(time.Unix(1_700_000_000, 0)) + stop := make(chan struct{}) + w := start(t, clk, idleExitOptions{threshold: time.Hour, act: &activityClock{}, + pending: func() int { return 0 }, stop: stop}) + close(stop) + select { + case r := <-w.res: + if r != "" { + t.Fatalf("stop should abandon the watch, got exit reason %q", r) + } + case <-time.After(3 * time.Second): + t.Fatal("watchIdle ignored stop") + } +} + +// TestIdleExitStartsItsClockAtLaunch: a proxy that never serves a single request still has to +// exit. Nothing stamps the activity clock in that case, so watchIdle has to seed it itself — +// a zero clock would otherwise read as "idle since 1970" and exit on the first tick, which is +// the opposite failure and just as wrong. +func TestIdleExitStartsItsClockAtLaunch(t *testing.T) { + clk := newFakeClock(time.Unix(1_700_000_000, 0)) + w := startWith(t, clk, idleExitOptions{threshold: time.Hour, act: &activityClock{}, + pending: func() int { return 0 }, stop: make(chan struct{})}, true) + w.mustNotExit("first tick on a proxy that has served nothing") + clk.advance(61 * time.Minute) + w.mustExit("an hour after launch with no traffic at all") +} + +// TestIdleCheckIntervalStaysUseful pins one rule and one cap: the watchdog looks every +// threshold/20 — 5% of what was asked for — until that would exceed five minutes. +// +// The third case here used to be labelled "clamped low" and asserted 10m -> 30s, which is exactly +// 10m/20: it passed whether or not the clamp existed, and the clamp it claimed to cover could +// never fire anyway, because checkIdleExit refuses any threshold under an hour. The clamp is gone +// and so is the case that pretended to test it. +func TestIdleCheckIntervalStaysUseful(t *testing.T) { + for _, c := range []struct { + threshold, want time.Duration + why string + }{ + {24 * time.Hour, 5 * time.Minute, "capped: 24h/20 is 72m, which would be an hour of slack"}, + {100 * time.Hour, 5 * time.Minute, "capped, well past the cap"}, + {time.Hour, 3 * time.Minute, "the floor: 5% of an hour, and the finest resolution reachable"}, + {80 * time.Minute, 4 * time.Minute, "threshold/20 while under the cap"}, + // Below the floor checkIdleExit refuses to start, so nothing here can be reached in + // production. Asserted anyway so the function stays total rather than surprising. + {10 * time.Minute, 30 * time.Second, "unreachable in production (below the floor)"}, + } { + if got := idleCheckInterval(c.threshold); got != c.want { + t.Errorf("idleCheckInterval(%s) = %s, want %s — %s", c.threshold, got, c.want, c.why) + } + } +} + +// TestProbesDoNotDeferIdleExit is the other half, and it is the one that was a live bug. +// +// A liveness probe or a Prometheus scrape is a machine asking whether the process is up — not +// somebody using it. Counting those did not weaken --idle-exit, it DISABLED it: any probe on a +// schedule shorter than the threshold means the exit never fires, and the only log line is the +// `idle-exit armed` one at startup, so nothing says it silently stopped working. Measured on a +// 1h-threshold proxy: 2h03m of wall clock, then `idle for 1h3m0s`, the clock having been held +// up for an hour by a /healthz poller alone. +func TestProbesDoNotDeferIdleExit(t *testing.T) { + clk := newFakeClock(time.Unix(1_700_000_000, 0)) + act := &activityClock{} + h := stampActivity(probeMux(), act, clk.now) + + w := start(t, clk, idleExitOptions{threshold: time.Hour, act: act, + pending: func() int { return 0 }, stop: make(chan struct{})}) + + // A probe every 10 minutes for two hours — the shape of a real monitoring loop. + for i := 0; i < 12; i++ { + clk.advance(10 * time.Minute) + for _, path := range []string{"/healthz", "/metrics"} { + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", path, nil)) + } + } + if got := w.mustExit("two hours of nothing but liveness probes"); got == "" { + t.Fatal("no exit reason") + } + + // And the asymmetry is deliberate, so pin it: a dashboard poll IS use. Exiting under + // somebody who is watching is a worse failure than a process left running. + clk2 := newFakeClock(time.Unix(1_700_000_000, 0)) + act2 := &activityClock{} + h2 := stampActivity(probeMux(), act2, clk2.now) + w2 := start(t, clk2, idleExitOptions{threshold: time.Hour, act: act2, + pending: func() int { return 0 }, stop: make(chan struct{})}) + clk2.advance(50 * time.Minute) + h2.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/api/events", nil)) + clk2.advance(30 * time.Minute) + w2.mustNotExit("a dashboard tab is open and polling") +} + +// TestCheckIdleExitRefusesAGatewaySelfTerminating covers the second startup refusal. +// +// `--upstreams` means this process serves other people's agents. A proxy that vanishes overnight +// there is a far worse failure than one left running on a laptop — and the protection used to be +// accidental: it held only because a hosted deployment runs a liveness probe, and every probe +// stamped the activity clock. probeRoutes deliberately stopped counting probes, which removes +// that accident, so the refusal has to be explicit or the combination silently becomes live. +func TestCheckIdleExitRefusesAGatewaySelfTerminating(t *testing.T) { + ok := store.Options{} // default TTL => floor 5h33m20s + good := 24 * time.Hour // clears the floor + for _, c := range []struct { + name string + d time.Duration + upstreams string + wantErr string + }{ + {"laptop install: no upstreams", good, "", ""}, + {"gateway with idle-exit", good, "/etc/context-guru/upstreams.yaml", "--upstreams"}, + // Off is always fine, including on a gateway: that is the shipped default and the + // refusal must not fire on a configuration everybody runs. + {"gateway without idle-exit", 0, "/etc/context-guru/upstreams.yaml", ""}, + // The floor still applies, and it is reported first — a threshold that is BOTH too short + // and on a gateway should name the floor, since that is the value the operator typed. + {"below the floor", 30 * time.Minute, "", "floor"}, + {"below the floor on a gateway", 30 * time.Minute, "/etc/x.yaml", "floor"}, + } { + err := checkIdleExit(c.d, c.upstreams, "", ok) + switch { + case c.wantErr == "" && err != nil: + t.Errorf("%s: refused a valid configuration: %v", c.name, err) + case c.wantErr != "" && err == nil: + t.Errorf("%s: accepted a configuration that must not start", c.name) + case c.wantErr != "" && err != nil && !strings.Contains(err.Error(), c.wantErr): + t.Errorf("%s: message does not mention %q: %v", c.name, c.wantErr, err) + } + } +} + +// TestActivityClockKeepsItsMonotonicReading is the guard for a defect that no other test here can +// see, because they all inject a fake clock built from time.Unix — which has no monotonic reading +// to lose. +// +// The clock stored `now.UnixNano()` and rebuilt the instant with `time.Unix(0, ns)`. That value +// carries no monotonic reading, so `now.Sub(act.last())` was wall-clock arithmetic: a laptop +// suspend/resume or an NTP step counts as idleness, and the watchdog can fire on its first tick +// after a lid-open, racing the user's first request. On the laptop this feature exists for, +// suspend is the normal case rather than an edge one. +// +// `t.Round(0)` strips the monotonic reading, and time.Time's == compares wall, monotonic and +// location — so `stored.Round(0) != stored` is precisely "this value still has a monotonic +// reading". +func TestActivityClockKeepsItsMonotonicReading(t *testing.T) { + var act activityClock + act.touch(time.Now()) + + stored := act.last() + if stored.Round(0) == stored { + t.Error("the stored instant has no monotonic reading, so idleness is measured against the " + + "wall clock: a suspend/resume or an NTP step is counted as idle time") + } + // And the subtraction the watchdog actually performs must stay monotonic end to end. + if elapsed := time.Now().Sub(act.last()); elapsed < 0 { + t.Errorf("elapsed since the stamp is negative (%s), which wall-clock arithmetic permits "+ + "and a monotonic reading does not", elapsed) + } +} + +// TestStampActivityRefreshesOnCompletion: a request that takes a while must not leave the clock +// reading from the moment it STARTED. +// +// Stamping only on entry meant a long request looked like a gap in use the moment it finished. The +// residual — that the clock is not refreshed DURING a request, so a single request outliving the +// whole threshold with no other traffic can still age out — is documented on stampActivity rather +// than fixed, and is unreachable in practice because the dashboard UI polls every 30s. +func TestStampActivityRefreshesOnCompletion(t *testing.T) { + clk := newFakeClock(time.Unix(1_700_000_000, 0)) + var act activityClock + mux := http.NewServeMux() + mux.HandleFunc("POST /anthropic/v1/messages", func(w http.ResponseWriter, r *http.Request) { + // The request takes 20 minutes of wall clock, as far as the injected clock is concerned. + clk.advance(20 * time.Minute) + w.WriteHeader(http.StatusOK) + }) + mux.HandleFunc("GET /healthz", func(w http.ResponseWriter, r *http.Request) {}) + h := stampActivity(mux, &act, clk.now) + + start := clk.now() + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("POST", "/anthropic/v1/messages", nil)) + + if got := act.last(); !got.After(start) { + t.Errorf("clock reads %s, the moment the request STARTED (%s) — a long request then looks "+ + "like 20 minutes of idleness the instant it completes", got, start) + } + if want := start.Add(20 * time.Minute); !act.last().Equal(want) { + t.Errorf("clock = %s, want the completion time %s", act.last(), want) + } + // A probe must still be stamped on neither edge. + before := act.last() + clk.advance(time.Hour) + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", "/healthz", nil)) + if !act.last().Equal(before) { + t.Errorf("a /healthz probe moved the clock to %s; probes count as neither entry nor "+ + "completion activity", act.last()) + } +} + +// TestParseEnvDurationRefusesAUnitlessValue: `IDLE_EXIT=86400` is the natural mistake for something +// documented as a duration, and it used to mean "never exit" — silently, because the +// `idle-exit armed` line is only logged for a value above zero, so the evidence was the ABSENCE of +// a log line. +func TestParseEnvDurationRefusesAUnitlessValue(t *testing.T) { + const def = 7 * time.Hour + for _, c := range []struct { + raw string + want time.Duration + wantErr bool + }{ + {"", def, false}, // not set + {" ", def, false}, // whitespace only + {"24h", 24 * time.Hour, false}, + {"30m", 30 * time.Minute, false}, + {"1500ms", 1500 * time.Millisecond, false}, + {"86400", 0, true}, // seconds, unitless — the reported mistake + {"24", 0, true}, // hours, unitless + {"forever", 0, true}, + } { + got, err := parseEnvDuration(c.raw, def) + if c.wantErr { + if err == nil { + t.Errorf("parseEnvDuration(%q) returned %s and no error; a typo must not silently "+ + "become a different configuration", c.raw, got) + } + continue + } + if err != nil { + t.Errorf("parseEnvDuration(%q): unexpected error %v", c.raw, err) + } else if got != c.want { + t.Errorf("parseEnvDuration(%q) = %s, want %s", c.raw, got, c.want) + } + } +} + +// probeMux is the route shape stampActivity is asked about: the two machine probes, one real API +// route, and the dashboard's SSE endpoint. Registered with methods, as the proxy registers them. +func probeMux() *http.ServeMux { + m := http.NewServeMux() + nop := func(w http.ResponseWriter, r *http.Request) {} + m.HandleFunc("GET /healthz", nop) + m.HandleFunc("GET /metrics", nop) + m.HandleFunc("GET /api/events", nop) + m.HandleFunc("POST /anthropic/v1/messages", nop) + return m +} + +// TestProbeExemptionSurvivesATrailingSlash is the hole an exact path compare left open, and it is +// the one that matters most because it fails SILENTLY in the safe-looking direction. +// +// A probe configured with a trailing slash — `GET /healthz/` — used to count as activity, so it +// refreshed the clock forever: --idle-exit never fired, and the only log line was `idle-exit armed` +// at startup. +// +// Be exact about WHY it is exempt now, because the first version of this comment was not: +// http.ServeMux does NOT redirect `/healthz/` to `/healthz`. cleanPath re-appends the trailing slash +// and matchOrRedirect only ever ADDS one, so with this route table `/healthz/` is a plain 404 and +// `Handler` reports the EMPTY pattern. That is what exempts it — the same branch that exempts +// `/nope`. (The redirect Go does generate, for a subtree root, also reports an empty pattern, so +// nothing here relies on a redirect resolving to its post-redirect pattern.) +// +// So every row below that is not a real route is exempt for one reason: no pattern matched. +func TestProbeExemptionSurvivesATrailingSlash(t *testing.T) { + for _, c := range []struct { + method, path string + isUse bool + why string + }{ + {"GET", "/healthz", false, "the probe itself"}, + {"GET", "/healthz/", false, "404, empty pattern — and a k8s probe spelled this way must not count"}, + {"GET", "/metrics", false, "a Prometheus scrape"}, + {"GET", "/metrics/", false, "404, empty pattern"}, + {"GET", "//healthz", false, "cleanPath collapses this to /healthz, which IS the probe pattern"}, + {"GET", "/health", false, "404 — a stray probe is not use"}, + {"GET", "/nope", false, "404 — a port scanner is not use"}, + {"GET", "/api/events", true, "the dashboard's SSE stream: a person is watching"}, + {"POST", "/anthropic/v1/messages", true, "an actual agent request"}, + } { + clk := newFakeClock(time.Unix(1_700_000_000, 0)) + var act activityClock + h := stampActivity(probeMux(), &act, clk.now) + + clk.advance(time.Minute) + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(c.method, c.path, nil)) + + stamped := !act.last().IsZero() + if stamped != c.isUse { + verb := "did not count" + if stamped { + verb = "counted" + } + t.Errorf("%s %s %s as activity, want the opposite — %s", c.method, c.path, verb, c.why) + } + } +} + +// TestCheckIdleExitKeepsTheOneHourMinimumWithNoStore: skipping the floor when the store is off +// removed it ENTIRELY, and a startup refusal became a startup PANIC. +// +// `STORE=false --idle-exit=10ns` passed the check, and threshold/20 then reached time.NewTicker as +// 0, which panics. Only the `2 x ttl_seconds` term is about the store; the bare 1h term is not, and +// dropping it also broke the invariant README and docs/reference/config.md state unconditionally. +func TestCheckIdleExitKeepsTheOneHourMinimumWithNoStore(t *testing.T) { + off := false + noStore := store.Options{Enabled: &off} + + // The 2x TTL term does not apply: ~5h34m would otherwise be the floor. + if err := checkIdleExit(2*time.Hour, "", "", noStore); err != nil { + t.Errorf("2h refused with the store disabled, where only the 1h minimum applies: %v", err) + } + // The 1h term still does. + for _, d := range []time.Duration{10 * time.Nanosecond, time.Millisecond, 30 * time.Minute, + time.Hour - time.Nanosecond} { + if err := checkIdleExit(d, "", "", noStore); err == nil { + t.Errorf("accepted --idle-exit=%s with the store disabled; anything under an hour is "+ + "shorter than the keep-alive's ping window, and a sub-second value cannot be "+ + "scheduled at all", d) + } + } + // Off is still always valid, and a gateway is still refused. + if err := checkIdleExit(0, "", "", noStore); err != nil { + t.Errorf("off refused: %v", err) + } + if err := checkIdleExit(24*time.Hour, "/etc/x.yaml", "", noStore); err == nil { + t.Error("a gateway with the store off may still not self-terminate") + } + + // And the store-off path must not become an escape hatch from the FULL floor: a store that is + // explicitly on, or simply unconfigured (which means on), is still protected by 2x the TTL. + on := true + if err := checkIdleExit(30*time.Minute, "", "", store.Options{Enabled: &on}); err == nil { + t.Error("accepted a threshold below the floor with the store explicitly enabled") + } + if err := checkIdleExit(30*time.Minute, "", "", store.Options{}); err == nil { + t.Error("accepted a threshold below the floor with the store unconfigured (which is on)") + } + // 2h clears the 1h minimum but not 2x the default TTL (~5h34m), so it must be refused when the + // store is on and accepted when it is off — that difference IS the store-off exemption. + if err := checkIdleExit(2*time.Hour, "", "", store.Options{}); err == nil { + t.Error("accepted 2h with the store on, where the floor is ~5h34m") + } +} + +// TestIdleCheckIntervalIsAlwaysPositive: time.NewTicker panics on a non-positive duration, and +// integer division makes zero reachable for a small enough threshold. checkIdleExit refuses those, +// so this cannot fire in production — but the previous version of the function reasoned exactly that +// way and was then reached with 10ns through a path that skipped the floor. A crash is the wrong +// failure mode for a helper, whatever its caller did. +func TestIdleCheckIntervalIsAlwaysPositive(t *testing.T) { + for _, d := range []time.Duration{0, 1, 5, 19 * time.Nanosecond, time.Nanosecond, time.Second} { + if got := idleCheckInterval(d); got <= 0 { + t.Errorf("idleCheckInterval(%s) = %s; time.NewTicker would panic", d, got) + } + } +} + +// TestCheckIdleExitRefusesBobModeToo closes the hole the third review found: --bob-upstream is a +// gateway flag too, and it was not covered. +// +// Two reasons it must be refused, and the second is the one that makes it urgent. First, Bob mode +// serves other people's agents, so self-terminating is as wrong there as with --upstreams. Second, +// proxy.Mux registers a `/` catch-all whenever BobUpstream is set — and with a catch-all present, +// mux.Handler answers "/" rather than the empty pattern for EVERY unmatched path, so `/healthz/`, +// `/nope` and every port-scan path count as activity and the watchdog never fires. Silently, with +// `idle-exit armed` as the only log line. +func TestCheckIdleExitRefusesBobModeToo(t *testing.T) { + good := 24 * time.Hour + for _, c := range []struct { + name, upstreams, bob string + wantRefused bool + }{ + {"laptop: neither", "", "", false}, + {"bob gateway", "", "https://api.us-east.bob.ibm.com", true}, + {"hosted gateway", "/etc/context-guru/upstreams.yaml", "", true}, + {"both", "/etc/context-guru/upstreams.yaml", "https://api.us-east.bob.ibm.com", true}, + } { + err := checkIdleExit(good, c.upstreams, c.bob, store.Options{}) + if c.wantRefused && err == nil { + t.Errorf("%s: accepted --idle-exit; a proxy with a `/` catch-all cannot also have a "+ + "watchdog, because every unmatched path would count as activity", c.name) + } + if !c.wantRefused && err != nil { + t.Errorf("%s: refused a valid laptop configuration: %v", c.name, err) + } + // The message must name the flag the operator actually passed, since that is the one they + // have to drop. + if c.wantRefused && err != nil { + want := "--upstreams" + if c.upstreams == "" { + want = "--bob-upstream" + } + if !strings.Contains(err.Error(), want) { + t.Errorf("%s: message does not name %s: %v", c.name, want, err) + } + } + } + // Off is always fine, in any mode. + if err := checkIdleExit(0, "", "https://api.us-east.bob.ibm.com", store.Options{}); err != nil { + t.Errorf("off refused in Bob mode: %v", err) + } +} + +// TestCatchAllRouteIsNotActivity is the belt to that braces. +// +// checkIdleExit now refuses --idle-exit alongside the flags that mount a `/` catch-all, so this +// combination is unreachable in a shipped configuration — but the two rules live in different files, +// and this is the one whose failure is silent. Over-counting a catch-all as "not use" errs toward +// exiting a laptop proxy; under-counting errs toward a gateway that never exits, which is the +// failure nobody notices. +func TestCatchAllRouteIsNotActivity(t *testing.T) { + clk := newFakeClock(time.Unix(1_700_000_000, 0)) + var act activityClock + + mux := http.NewServeMux() + nop := func(w http.ResponseWriter, r *http.Request) {} + mux.HandleFunc("GET /healthz", nop) + mux.HandleFunc("POST /anthropic/v1/messages", nop) + // Bob mode's catch-all, and the explicit Bob route beside it. + mux.HandleFunc("POST /inference/v1/chat/completions", nop) + mux.HandleFunc("/", nop) + h := stampActivity(mux, &act, clk.now) + + for _, c := range []struct { + method, path string + isUse bool + why string + }{ + {"GET", "/healthz", false, "the probe itself"}, + {"GET", "/healthz/", false, "falls through to the catch-all, and must still not count"}, + {"GET", "/nope", false, "a port scanner matching `/` is not somebody using the proxy"}, + {"POST", "/anthropic/v1/messages", true, "a real agent request"}, + {"POST", "/inference/v1/chat/completions", true, "Bob's own model route is explicit"}, + } { + act = activityClock{} + clk.advance(time.Minute) + h.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(c.method, c.path, nil)) + if stamped := !act.last().IsZero(); stamped != c.isUse { + verb := "did not count" + if stamped { + verb = "counted" + } + t.Errorf("with a `/` catch-all registered, %s %s %s as activity, want the opposite — %s", + c.method, c.path, verb, c.why) + } + } +} diff --git a/cmd/context-guru-proxy/main.go b/cmd/context-guru-proxy/main.go index f2eeeaa0..6a23de4a 100644 --- a/cmd/context-guru-proxy/main.go +++ b/cmd/context-guru-proxy/main.go @@ -63,7 +63,13 @@ func listenAndAnnounce(addr string, attrs ...any) (net.Listener, error) { func main() { var ( - addr = envOr("LISTEN_ADDR", ":4000") + // --listen, not just LISTEN_ADDR. Two reasons beyond taste: an operator reading `ps` + // could not tell which port an instance held (the address reached it only through the + // environment), and a supervisor that needs to stop ONE instance among several had + // nothing in the command line to match on. Pattern-matching a process for shutdown is + // still the wrong tool — but when it happens, the port must at least be visible. + addrFlag = flag.String("listen", envOr("LISTEN_ADDR", ":4000"), "address to listen on") + showVer = flag.Bool("version", false, "print version and exit") cfgPath = flag.String("config", envOr("CONFIG", ""), "path to context-guru YAML config") preset = flag.String("preset", envOr("PRESET", "house"), "preset to use when --config is absent (house = the service default, deterministic; housellm = the same plus the compaction-model pass; codesmart/codesafe = the SWE-bench study's configs, kept so its published numbers stay reproducible)") openai = flag.String("openai-upstream", envOr("OPENAI_UPSTREAM", "https://api.openai.com"), "OpenAI upstream base URL") @@ -78,6 +84,19 @@ func main() { bob = flag.String("bob-upstream", envOr("BOB_UPSTREAM", ""), "Bob (BobShell) backend base URL; enables the Bob gateway routes when set (e.g. https://api.us-east.bob.ibm.com)") storeFlag = flag.String("store", envOr("STORE", ""), "override state store: true|false (default: config store.enabled, else on)") modeFlag = flag.String("mode", envOr("MODE", ""), "operating mode: sync (default) | observe (overrides the config's mode:)") + // OFF by default, and it must stay that way: a gateway or eval-containers deployment + // that self-terminates is a much worse failure than a laptop process left running. + // + // It is meant to be paired with something that starts the proxy again on demand — the + // Claude Code plugin's SessionStart hook does that, and self-kill without a + // resurrection path is a footgun. That hook is NOT in this change: it ships with the + // plugin. Until then, anyone setting this by hand is choosing a proxy that will exit + // and stay exited, and the docs say so rather than implying a pairing that is not here. + // + // The floor and the gateway refusal are enforced in checkIdleExit, not documented. + idleExit = flag.Duration("idle-exit", envDuration("IDLE_EXIT", 0), + "exit after this long with no requests and no keep-alive ping pending (0 = never; "+ + "must be at least 2x store.ttl_seconds, see store.IdleExitFloor)") // Dashboard. Off by default so an existing deployment's behavior and route // table are unchanged until asked for; on, it adds /dashboard/ + /api/*. @@ -190,11 +209,25 @@ func main() { ) flag.Parse() + // --version before anything else: an installer asks a binary what it is, and it must be + // able to ask without starting a server or needing a config. buildinfo.Version was already + // compiled in and reachable only via /stats, which requires a running proxy — so + // `context-guru-proxy --version` was answered by the flag package's usage text, and an + // installer parsing it recorded "Usage of context-guru-proxy:" as the installed version. + if *showVer { + fmt.Printf("context-guru-proxy %s (commit %s)\n", buildinfo.Version, buildinfo.Commit) + return + } + addr := *addrFlag + // Logging first, before anything can want to log. Level, format and sink come from // the environment (CG_LOG_LEVEL / CG_LOG_FORMAT / CG_LOG_FILE / CG_LOG_PLAIN) rather // than flags, because the two places that set them are a systemd drop-in and a shell, // and both already speak environment. See internal/logging. sink := logging.Setup() + // Now that --version has returned and the log sink exists, refuse any malformed duration the + // var(...) block recorded. Deliberately before the config load and before anything is opened. + checkEnvDurations() cfg, err := loadConfig(*cfgPath, *preset) if err != nil { @@ -211,6 +244,19 @@ func main() { cfg.Store.Enabled = &v // flag/env wins over the config file when set } + // Refuse a bad --idle-exit HERE, immediately after the config is resolved and before + // anything is opened. + // + // It moved twice. First it sat after the "listening" line, so a rejected configuration read + // as a crash. Then it sat after the dashboard and control databases are opened — and + // log.Fatalf calls os.Exit, which runs no defers, so a first-time user who typed + // `--idle-exit 30m` got both SQLite files created and migrated and then an abrupt exit with + // WAL/-shm left behind. Everything the check reads (the flags, cfg.Store) is known right + // here, so the refusal costs nothing and leaves nothing behind. + if err := checkIdleExit(*idleExit, *upstreamsPath, *bob, cfg.Store); err != nil { + log.Fatalf("context-guru: %v", err) + } + agg := metrics.NewAggregator() // metrics.Slog is deliberately NOT wired in here any more. It emitted one line per // component plus one per run, at INFO, with no tenant and no session on any of them — @@ -578,9 +624,23 @@ func main() { log.Fatalf("listen: %v", err) } + // Activity stamping is wired ONLY when the watchdog is on, so an ordinary deployment's + // handler chain is byte-identical to before. + mux := h.Mux() + var handler http.Handler = mux + act := &activityClock{} + if *idleExit > 0 { + // Launch counts as activity, so the threshold is measured from a moment that means + // something rather than from whenever the watchdog goroutine is first scheduled. + act.touch(time.Now()) + handler = stampActivity(mux, act, time.Now) + slog.Info("context-guru: idle-exit armed", "after", *idleExit, + "check_every", idleCheckInterval(*idleExit)) + } + srv := &http.Server{ Addr: addr, - Handler: h.Mux(), + Handler: handler, // ReadHeaderTimeout is the one that matters for a service on a network: without // it, a client that opens a connection and never finishes its headers holds a // goroutine and a file descriptor indefinitely. @@ -600,12 +660,36 @@ func main() { // Graceful shutdown, so the dashboard's writer goroutine flushes its batch and any // in-flight archive upload is not abandoned halfway. Without this, a restart loses // the last few hundred milliseconds of captured requests every time. + // + // Both reasons to stop — a signal, and the idle watchdog — converge on ONE teardown, so + // the self-terminating path cannot drift from the one that is known to work. idle := make(chan struct{}) + why := make(chan string, 2) + stopWatch := make(chan struct{}) go func() { sig := make(chan os.Signal, 1) signal.Notify(sig, syscall.SIGINT, syscall.SIGTERM) s := <-sig - slog.Info("context-guru: shutting down", "signal", s.String()) + why <- "signal " + s.String() + }() + if *idleExit > 0 { + t := time.NewTicker(idleCheckInterval(*idleExit)) + go func() { + defer t.Stop() + // h.PendingPings is the half of "idle" that requests cannot express: the + // keep-alive works precisely when no client traffic is arriving. + if reason, ok := watchIdle(idleExitOptions{ + threshold: *idleExit, act: act, pending: h.PendingPings, + now: time.Now, tick: t.C, stop: stopWatch, + }); ok { + why <- reason + } + }() + } + go func() { + reason := <-why + close(stopWatch) + slog.Info("context-guru: shutting down", "reason", reason) ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) defer cancel() if err := srv.Shutdown(ctx); err != nil { @@ -947,12 +1031,54 @@ func envInt(key string, def int) int { return def } -// envDuration reads a Go duration environment variable (e.g. "72h"). +// badDurations collects environment variables whose value is not a duration, so the refusal can +// happen at a moment where it is safe to refuse. See checkEnvDurations. +var badDurations []string + +// envDuration reads a duration from the environment, falling back to def. +// +// A NON-EMPTY value that does not parse is REFUSED — but not here, and that distinction is the +// whole point. Every call site is a default expression in main's var(...) block, which Go evaluates +// before flag.Parse, so exiting from inside this function ran before the --version short-circuit +// existed to be reached: `IDLE_EXIT=86400 context-guru-proxy --version` died with a parse error +// instead of printing the version, which is exactly what an installer asks for and what the release +// workflow greps. It also ran before logging.Setup(), so the message never reached CG_LOG_FILE. +// +// So the value is recorded and checkEnvDurations refuses later, past --version and past the log +// sink. Refusing at all — rather than silently falling back, which is what this used to do — is +// still the right behaviour: every caller is a timeout, a retention window or a process lifetime, +// and `IDLE_EXIT=86400` meant "never exit" with no output whose absence anyone would notice. func envDuration(key string, def time.Duration) time.Duration { - if d, err := time.ParseDuration(strings.TrimSpace(os.Getenv(key))); err == nil { - return d + d, err := parseEnvDuration(os.Getenv(key), def) + if err != nil { + badDurations = append(badDurations, + fmt.Sprintf("%s=%q (%v)", key, strings.TrimSpace(os.Getenv(key)), err)) + return def } - return def + return d +} + +// checkEnvDurations refuses every malformed duration at once, or returns. +// +// All of them, not the first: an operator fixing a typo should not have to restart to discover the +// next one. Called after flag.Parse and after the log sink is up, so `--version` and `--help` still +// work and the message lands wherever the logs go. +func checkEnvDurations() { + if len(badDurations) == 0 { + return + } + log.Fatalf("context-guru: not a duration: %s. Use a unit — 24h, 30m, 1500ms — or unset it to "+ + "accept the default.", strings.Join(badDurations, "; ")) +} + +// parseEnvDuration is envDuration's decision, split out so it can be tested without a process +// that calls os.Exit. Empty means "not set" and yields the default; anything else must parse. +func parseEnvDuration(raw string, def time.Duration) (time.Duration, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return def, nil + } + return time.ParseDuration(raw) } func envOr(key, def string) string { diff --git a/config/config.go b/config/config.go index 4bc6c0b0..f554fb38 100644 --- a/config/config.go +++ b/config/config.go @@ -358,7 +358,27 @@ func (c *Config) applyPreset() error { // sweep found 0 convertible candidates in 11.67M tokens. It was costing 1.53 ms and a // TextTokens call per tool message to convert nothing. var presets = map[string][]string{ - "off": {}, // passthrough: no components (baseline / A-B control) + "off": {}, // passthrough: no components (baseline / A-B control) + // cache: the volatile-tail split and NOTHING else. This is the preset a stranger + // evaluating context-guru on their own Claude Code sessions is pointed at, and the + // reason it exists is that it can be verified by reading this one line: no content is + // dropped, no `<>` marker is written, no expand tool is injected into the + // request, and no model is called. The loudest objection to putting a proxy on the + // wire — "you are editing my agent's context" — does not apply to it. + // + // It is also the best-evidenced single component in the repo: -34.1% cost and 0% -> + // 96.7% prefix-cache hit in an isolated A/B (docs/results/context-guru.md), which is + // why the funnel leads with the cache rather than the offloaders. + // + // Deliberately NOT `safe` (format -> textclean -> searchfold -> cachesplit): those are + // lossless in meaning but they still rewrite the JSON, so "we do not touch your + // context" stops being literally true and a reviewer has to take four components on + // trust instead of reading one. TestCachePresetIsCachesplitAlone holds it to that, and + // the lossless-folds rule exempts it for the same reason. + // + // Anthropic-family only, and the docs say so: cachesplit is a no-op on implicit + // prefix-cache backends (vLLM, llm-d) — see apply/prefixsplit.go. + "cache": {"cachesplit"}, "safe": {"format", "textclean", "searchfold", "cachesplit"}, "balanced": {"format", "textclean", "searchfold", "dedup", "failed_run", "cmdfilter", "linecap", "cachesplit"}, "aggressive": {"format", "textclean", "searchfold", "dedup", "failed_run", "cmdfilter", "smartcrush", "extract", "extract_llm", "linecap", "cachesplit"}, diff --git a/config/config_more_test.go b/config/config_more_test.go index 5af1c4ed..26ffc5fa 100644 --- a/config/config_more_test.go +++ b/config/config_more_test.go @@ -125,7 +125,13 @@ func TestLosslessFoldsAreInEveryWorkingPreset(t *testing.T) { // restructures the transcript alone, and agentdiet reproduces a published baseline // whose whole claim is what ONE reflection achieves — stacking folds beside it would // reduce the same outputs first and there would be nothing left to attribute. - exempt := map[string]bool{"off": true, "summarize": true, "agentdiet": true} + // `cache` is exempt for a reason the other three do not share: its whole product claim + // is that it is ONE component, verifiable by reading one line of the presets map. Adding + // format/textclean/searchfold to it would each be lossless in meaning and would still + // cost the claim — a stranger deciding whether to route their agent through us can check + // "nothing but a cache breakpoint moves" in a second, and cannot check four rewriters as + // fast. See TestCachePresetIsCachesplitAlone, which holds the other side of that trade. + exempt := map[string]bool{"off": true, "summarize": true, "agentdiet": true, "cache": true} for name, pipeline := range presets { if exempt[name] { continue @@ -217,3 +223,36 @@ func TestLinecapRunsLastAmongTheOffloaders(t *testing.T) { } } } + +// TestCachePresetIsCachesplitAlone guards the one preset whose CONTENT is its promise. +// +// `cache` is what the local-distribution funnel points a stranger at, and the pitch is +// exact: no content dropped, no `<>` marker written, no expand tool injected, no +// model called. That is not a property of cachesplit that survives company — every other +// component in the repo either rewrites JSON, offloads content, or calls a model, so ANY +// addition here converts a checkable claim into a trust-me claim, and the docs that make the +// claim (docs/how-to/choose-a-preset.md, the plugin's install skill) do not get to notice. +// +// It is also why `cache` is exempt from TestLosslessFoldsAreInEveryWorkingPreset. That +// exemption is only defensible while this test exists: without it, "cache is exempt from the +// folds rule" would read as permission to put anything at all in it. +func TestCachePresetIsCachesplitAlone(t *testing.T) { + p, ok := presets["cache"] + if !ok { + t.Fatal("preset `cache` is gone; the local-distribution funnel and the install skill both name it") + } + if len(p) != 1 || p[0] != "cachesplit" { + t.Fatalf("preset `cache` = %v, want exactly [cachesplit]: it is the only preset whose "+ + "losslessness is verifiable by reading one line, and every other component either "+ + "rewrites JSON, offloads content, or calls a model", p) + } + // The pipeline the proxy actually builds, not just the map literal: applyPreset and the + // rich-preset path both sit between this map and the wire. + built, ok := PresetPipeline("cache") + if !ok { + t.Fatal(`PresetPipeline("cache") did not resolve, so ?preset=cache would 400`) + } + if len(built) != 1 || built[0] != "cachesplit" { + t.Fatalf(`PresetPipeline("cache") = %v, want [cachesplit]`, built) + } +} diff --git a/docs/get-started/quickstart-proxy.md b/docs/get-started/quickstart-proxy.md index e82ebb44..597f605a 100644 --- a/docs/get-started/quickstart-proxy.md +++ b/docs/get-started/quickstart-proxy.md @@ -3,19 +3,31 @@ Run context-guru in front of your provider and point an agent at it. One port serves both the OpenAI and Anthropic dialects. -You need **Go 1.26**. You do **not** need a C toolchain: `make build` builds with cgo disabled, and -the result is a statically linked binary with no runtime dependencies. Everything else is a normal -module dependency — build straight from the repo root. +**You need no toolchain at all to run it.** The shipped binary is statically linked pure Go — no C +compiler, no Go install, no runtime dependencies. Grab it from +[Releases](https://github.com/rossoctl/context-guru/releases): + +```sh +# Pick your platform: linux/darwin × amd64/arm64. The archive unpacks into its own +# directory, so this is safe to run from anywhere — including a project checkout. +tar xzf context-guru_*_darwin_arm64.tar.gz +install -m 755 context-guru_*/context-guru-proxy ~/.local/bin/ +``` + +To build from source instead you need **Go 1.26** — and still no C toolchain: `make build` builds +with cgo disabled and produces the same statically linked binary. CI asserts that natively for +linux/amd64 (the `purego` job), and the release workflow asserts it again before publishing. A C compiler is needed for exactly two things: `make test` (the race detector requires cgo) and the optional [`skeleton`](../components/skeleton.md) component's `cg_skeleton` build tag. ## Steps -1. Build: +1. Build (source path only — skip if you downloaded a release): ```sh make build # → bin/context-guru-proxy + make build-static # the pure-Go build releases ship (CGO_ENABLED=0) ``` 2. Run it. It listens on `:4000`; set `LISTEN_ADDR` to change that. diff --git a/docs/how-to/choose-a-preset.md b/docs/how-to/choose-a-preset.md index 313f6f15..0451229f 100644 --- a/docs/how-to/choose-a-preset.md +++ b/docs/how-to/choose-a-preset.md @@ -11,6 +11,7 @@ context-guru-proxy --preset codesmart # or PRESET=codesmart, or preset: in | Your workload | Preset | |---|---| +| **Trying context-guru for the first time** | **`cache`** | | **Most agents — the recommended pipeline** | **`codesmart`** (pass `--preset codesmart`; the binary defaults to `house`) | | Same, but no LLM on the hot path | `codesafe` | | A guaranteed-safe, lossless win only | `safe` | @@ -28,6 +29,7 @@ context-guru-proxy --preset codesmart # or PRESET=codesmart, or preset: in |---|---| | `codesmart` | `format, textclean, searchfold, dedup, failed_run, cmdfilter, extract_llm, extract, linecap, cachesplit` | | `codesafe` | `format, textclean, searchfold, dedup, failed_run, cmdfilter, extract, collapse, linecap, cachesplit` | +| `cache` | `cachesplit` | | `safe` | `format, textclean, searchfold, cachesplit` | | `balanced` | `format, textclean, searchfold, dedup, failed_run, cmdfilter, linecap, cachesplit` | | `aggressive` | `format, textclean, searchfold, dedup, failed_run, cmdfilter, smartcrush, extract, extract_llm, linecap, cachesplit` | @@ -56,6 +58,39 @@ table so it lists every preset that exists; pick from the table above this one. ## Notes on the ones people pick +### `cache` — start here + +`cachesplit` and nothing else. Pick it when what you want is to find out whether this thing +helps you, with the smallest possible claim to check: + +- **Nothing is dropped, summarised, or replaced.** No `<>` markers, no + `context_guru_expand` tool added to your requests, no model calls. It splits one oversized + system block into two adjacent text blocks whose concatenation is byte-identical, so the + model sees exactly the prompt your agent sent — and moves the cache breakpoint onto the + half that does not churn. +- **What it is worth is regime-dependent, and the funnel's regime is the weak one.** The + headline **−34.1% cost / 0% → 96.7% hit** comes from a benchmark harness running tasks + back-to-back inside the provider's 5-minute cache TTL + ([cacheinject](../components/cacheinject.md#what-the-split-is-worth)), which is + precisely the regime where the split pays — and is *one task measured three times, not a + fleet average*. On this project's own interactive traffic the figure is **$0.0298 across + 1,127 sessions / 11,361 requests** + ([dashboard](../dashboard.md#what-it-is-actually-worth-here-and-why-that-is-small)): Claude + Code captures the environment snapshot once per session, and 1,105 of 1,127 session starts + read zero tokens from cache because the previous prefix had already expired. It is also + **exactly zero** outside a git repository, on a system prompt under the 1,024-token + `minSplitTokens` floor, and on any implicit prefix-cache backend (vLLM, llm-d). Neither + figure is wrong; they differ by three orders of magnitude because the mechanism needs a + second session inside five minutes. + +- **Anthropic-family only.** `cachesplit` is a no-op against implicit prefix-cache backends + (vLLM, llm-d), which match to the divergence on their own — so on those it costs nothing + and buys nothing. + +Move to `codesmart` once you want the offloaders too. `safe` is the next step up and is still +lossless in meaning, but it does rewrite JSON, so `cache` is the one whose promise you can +confirm by reading a single line of `config/config.go`. + **`codesmart`** is the shipped default and the cheapest arm in the [benchmarks](../RESULTS.md) at the highest reward. It is the one preset that ships tuned per-component settings rather than a bare name-list, which is why most turns make no model diff --git a/docs/how-to/use-with-claude-code.md b/docs/how-to/use-with-claude-code.md index ff3eaefd..d749d95a 100644 --- a/docs/how-to/use-with-claude-code.md +++ b/docs/how-to/use-with-claude-code.md @@ -3,6 +3,30 @@ Route [Claude Code](https://docs.claude.com/en/docs/claude-code) through context-guru with one environment variable — no changes to Claude Code itself. +## You do not need an API key + +Setting `ANTHROPIC_BASE_URL` **without** a credential variable leaves your claude.ai login in +place: a Pro or Max subscription keeps working, with your usage limits and billing unchanged. You +can run context-guru in front of your own sessions with **no API key at all** — which is the +cheapest way to evaluate it. + +Two honest caveats: + +- On subscription billing the saving lands in **usage limits**, not dollars, so `/stats` cost + figures are list-price estimates and will not match a bill you do not receive. +- Setting `ANTHROPIC_API_KEY` or `ANTHROPIC_AUTH_TOKEN` in your Claude Code environment is what + moves you onto metered API billing. Only do it deliberately — see + [Keep the API key out of Claude Code](#keep-the-api-key-out-of-claude-code), which is about + the *proxy* holding the key, not Claude Code. +- **"No API key" does not mean the proxy sees less.** Routing subscription-authenticated Claude Code + through it means the proxy receives your claude.ai OAuth credential on every request and forwards + it upstream — that is what keeps your subscription working. If the prompt-cache keep-alive is + enabled, the proxy also RETAINS that credential in memory for the life of a tracked session, so it + can replay a request on your behalf; those pings are billed to you, spending the same usage limits + as your own turns. The credential is zeroised when the entry is dropped + ([keep-alive](cache-keepalive.md)) and never written to disk, but a local proxy holding a live + credential is the trade being made, and the section below is about a different one. + ## Steps 1. Start the proxy: @@ -38,6 +62,9 @@ Add to `.claude/settings.json` so you don't export anything by hand: } ``` +Use `.claude/settings.local.json` instead if you do not want to commit it: a base URL pointing at +`localhost` breaks Claude Code for everyone who clones the repo whenever the proxy is not running. + ## Keep the API key out of Claude Code Give the proxy the real key and hand Claude Code a placeholder; the proxy injects the diff --git a/docs/reference/config.md b/docs/reference/config.md index 682ea692..a6c3fd09 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -106,7 +106,9 @@ for every component's config block. |---|---|---| | `--preset` / `PRESET` | `house` | Pipeline preset when no `--config`. `codesmart` is the SWE-bench arm and must be asked for by name. | | `--config` / `CONFIG` | — | YAML config file (overrides preset). | -| `LISTEN_ADDR` | `:4000` | Listen address. | +| `--listen` / `LISTEN_ADDR` | `:4000` | Listen address. The flag exists so the port is visible in `ps` and to a supervisor; before it, the address reached the process only through the environment. | +| `--version` | — | Print version and commit, then exit. | +| `--idle-exit` / `IDLE_EXIT` | `0` (never) | Exit after this long with **no requests and no keep-alive ping pending**, so a proxy started on demand does not outlive its use. Refused at startup below `max(2 × store.ttl_seconds, 1h)` — 5h33m20s at the default TTL — because exiting clears the in-memory store, and losing a frozen decision re-bills its whole prefix as cache creation. Also refused together with `--upstreams`: a gateway serving other people's agents must not self-terminate. Liveness probes (`/healthz`, `/metrics`) deliberately do **not** count as activity; anything else does, including the dashboard's own polling. | | `--openai-upstream` / `OPENAI_UPSTREAM` | `https://api.openai.com` | OpenAI upstream base. | | `--anthropic-upstream` / `ANTHROPIC_UPSTREAM` | `https://api.anthropic.com` | Anthropic upstream base. | | `--bob-upstream` / `BOB_UPSTREAM` | — | Bob (BobShell) backend base. Setting it mounts the [Bob gateway routes](routes.md#bob-bobshell-gateway-routes); unset, an unknown path 404s as before. | diff --git a/docs/reference/presets.md b/docs/reference/presets.md index c6a00f84..06d82a13 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -12,6 +12,7 @@ taken exactly from the `presets` map in `config/config.go`. | `codesmart` | `format` → `textclean` → `searchfold` → `dedup` → `failed_run` → `cmdfilter` → `extract_llm` → `extract` → `linecap` → `cachesplit` | The SWE-bench-winning cache-aware config: structural offloaders + a cheap-model relevance-trimmer (`extract_llm`, routed to `CHEAP_MODEL`, gated so most turns make no model call) + deterministic `extract`. `extract_llm` no-ops (→ deterministic) when no cheap model is configured. **Changed 2026-08:** the lossless trio replaced `toon`, which acted 0 times on 5,752 production requests, and `linecap` was added. Re-measure before quoting the published SWE-bench numbers against it. | | `codesafe` | `format` → `textclean` → `searchfold` → `dedup` → `failed_run` → `cmdfilter` → `extract` → `collapse` → `linecap` → `cachesplit` | `codesmart` minus the LLM pass — **deterministic-only, zero model calls by policy**. The safe control / the choice when you don't want an LLM on the hot path. | | `off` | *(empty)* | Passthrough — no components. The baseline / A-B control. | +| `cache` | `cachesplit` | **The first-run preset** — the one to point a new evaluator at. The volatile-tail split and nothing else: no content dropped, no `<>` markers, no `context_guru_expand` tool added to requests, no model calls. Chosen so a stranger deciding whether to route their agent through a local proxy can verify the claim by reading one line of `config/config.go` rather than trusting four components. The savings claim is regime-dependent and the funnel's regime is the weak one: **−34.1% cost / 0% → 96.7% hit** is a benchmark harness running tasks back-to-back inside the provider's 5-minute TTL (and is one task measured three times), while this project's own interactive traffic yields **$0.0298 across 1,127 sessions** — 1,105 of 1,127 session starts read zero from cache. Zero outside a git repo, under the 1,024-token `minSplitTokens` floor, or on an implicit prefix-cache backend (vLLM, llm-d). See [dashboard](../dashboard.md#what-it-is-actually-worth-here-and-why-that-is-small) and [cacheinject](../components/cacheinject.md). | | `safe` | `format` → `textclean` → `searchfold` → `cachesplit` | Lossless only: repack JSON compactly and split the volatile system tail so the shared prefix stays cacheable. Zero risk of dropping content. | | `balanced` | `format` → `textclean` → `searchfold` → `dedup` → `failed_run` → `cmdfilter` → `linecap` → `cachesplit` | Lossless repack + conservative offloads (dedupe, drop superseded/failed runs, filter command noise) + the cache split. **Not recommended for agentic traffic** — it omits `mask`, the biggest lever there. | | `aggressive` | `format` → `textclean` → `searchfold` → `dedup` → `failed_run` → `cmdfilter` → `smartcrush` → `extract` → `extract_llm` → `linecap` → `cachesplit` | `balanced` plus `smartcrush` (crush long homogeneous arrays), deterministic `extract` (noise collapse), and `extract_llm` (cheap-model relevance trim) for deeper savings. | diff --git a/docs/setup.md b/docs/setup.md index aa63ef66..3c21e35a 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -10,12 +10,14 @@ SWE-bench task driven by Claude Code. pure Go and statically linked. bifrost's tokenizer does **not** use cgo: o200k_base is embedded (`internal/tokens/tokens.go`). CI asserts the pure-Go build on every PR — natively, for linux/amd64 — in the `purego` job (`.github/workflows/ci.yaml`), which builds with - `CGO_ENABLED=0`, checks the artifact is statically linked, starts it and probes `/healthz`. So the - claim cannot rot back into a false one for the platform CI runs on. + `CGO_ENABLED=0`, checks the artifact is statically linked, starts it and probes `/healthz`. The + release workflow asserts it again before publishing, deliberately: a release must not depend on a + PR check having run. Cross-compilation to the other three release targets (linux/arm64, darwin/amd64, darwin/arm64) is - **not** covered by that job: it was verified by hand on go 1.26.4 and is asserted at release time - by the tag workflow, not per PR. + covered by the release build, not by that per-PR job. +- If you do not need `skeleton`, skip the build entirely and use a + [release binary](https://github.com/rossoctl/context-guru/releases). - **Docker** (for the gateway image / eval-containers), and the **eval-containers** repo. ## Build diff --git a/proxy/conformance_test.go b/proxy/conformance_test.go new file mode 100644 index 00000000..084ad9a9 --- /dev/null +++ b/proxy/conformance_test.go @@ -0,0 +1,307 @@ +package proxy_test + +import ( + "bufio" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/tidwall/gjson" +) + +// Gateway conformance under the funnel's default preset. +// +// The local-distribution funnel puts context-guru on the wire in front of a stranger's Claude +// Code, so "it works" is not enough — it has to not break the client, and the ways it could are +// specific and documented in the gateway protocol reference. Each test below is one of them. +// +// Two of these are worse than an outage, because they make the demo read as NEGATIVE rather +// than broken: +// +// - a buffered SSE response looks like the proxy made the model slow; +// - a rejected `cache_control` marker makes Claude Code disable prompt caching for the rest +// of the conversation, which switches off the exact thing being sold, silently. +// +// The preset under test is `cache` (cachesplit alone) throughout, because that is what an +// evaluator actually runs. The shared Claude-Code-shaped fixtures are in ccbody_test.go, and the +// two items that were DEFECTS rather than confirmations — the expand-tool gate and the missing +// count_tokens route — are tested beside their fixes in expandgate_test.go and +// counttokens_test.go. +// TestCachePresetForwardsAnUpstreamErrorByteForByte covers conformance item 4. +// +// Claude Code's capability-rejection recovery matches on the upstream's error WORDING. A gateway +// that wraps, re-encodes or summarises an error body breaks that recovery path — the client can +// no longer tell "your cache_control was refused" from any other 400, so instead of retrying +// without the capability it surfaces a failure. The status, the body and the content type all +// have to arrive exactly as the upstream wrote them. +func TestCachePresetForwardsAnUpstreamErrorByteForByte(t *testing.T) { + // A real Anthropic error shape, whitespace and key order included: this is what the + // client's matching runs against, so the test compares bytes rather than parsed JSON. + errBody := `{"type":"error","error":{"type":"invalid_request_error","message":"A maximum of 4 blocks with cache_control may be provided, but found 5."}}` + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("request-id", "req_upstream_123") + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, errBody) + })) + defer upstream.Close() + + h, _ := buildHandler(t, cachePipeline, upstream.URL) + srv := httptest.NewServer(h.Mux()) + defer srv.Close() + + resp, err := http.Post(srv.URL+"/anthropic/v1/messages", "application/json", + strings.NewReader(string(claudeCodeBody(t, false)))) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + got, _ := io.ReadAll(resp.Body) + + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("status = %d, want 400: a rewritten status breaks the client's retry logic", + resp.StatusCode) + } + if string(got) != errBody { + t.Errorf("the error body was modified.\n got: %s\nwant: %s\n"+ + "Claude Code matches on the upstream's own wording to decide whether to retry "+ + "without a capability; wrapping it disables that recovery.", got, errBody) + } + if resp.Header.Get("request-id") != "req_upstream_123" { + t.Errorf("request-id header lost (%q): it is what support uses to find the call", + resp.Header.Get("request-id")) + } +} + +// TestCachePresetDoesNotBufferSSE covers conformance item 1. +// +// Claude Code aborts a stream that has been silent for 300s, and a gateway that buffers a whole +// response before relaying it stalls the client. context-guru does buffer SOME responses — the +// ones where the model opens by calling the expand tool — but under the `cache` preset nothing +// injects that tool, so the buffering path must be unreachable. This asserts that rather than +// assuming it: the client's first event has to arrive while the upstream is still writing later +// ones. +func TestCachePresetDoesNotBufferSSE(t *testing.T) { + release := make(chan struct{}) + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + fl, _ := w.(http.Flusher) + fmt.Fprint(w, "event: message_start\ndata: {\"type\":\"message_start\",\"message\":{\"usage\":{\"input_tokens\":10}}}\n\n") + if fl != nil { + fl.Flush() + } + // Hold the rest of the stream until the test has SEEN the first event. If the proxy + // buffered, the read below would block here and the test fails on the deadline rather + // than on a wrong byte — which is exactly the client-visible symptom. + <-release + fmt.Fprint(w, "event: message_stop\ndata: {\"type\":\"message_stop\"}\n\n") + if fl != nil { + fl.Flush() + } + })) + defer upstream.Close() + + h, _ := buildHandler(t, cachePipeline, upstream.URL) + srv := httptest.NewServer(h.Mux()) + defer srv.Close() + + req, _ := http.NewRequest("POST", srv.URL+"/anthropic/v1/messages", + strings.NewReader(string(claudeCodeBody(t, true)))) + req.Header.Set("Content-Type", "application/json") + resp, err := (&http.Client{Timeout: 15 * time.Second}).Do(req) + if err != nil { + close(release) + t.Fatal(err) + } + defer resp.Body.Close() + + type read struct { + line string + err error + } + ch := make(chan read, 1) + go func() { + line, err := bufio.NewReader(resp.Body).ReadString('\n') + ch <- read{line, err} + }() + select { + case r := <-ch: + close(release) + if r.err != nil { + t.Fatalf("reading the first event: %v", r.err) + } + if !strings.Contains(r.line, "message_start") { + t.Fatalf("first line was %q, want the upstream's first event", r.line) + } + case <-time.After(5 * time.Second): + close(release) + t.Fatal("no event reached the client while the upstream was still streaming: the " + + "response is being buffered. Claude Code aborts a stream silent for 300s, and a " + + "stalled first byte reads as context-guru making the model slow.") + } +} + +// TestCachePresetNeverAddsACacheControlBreakpoint covers conformance item 2, which is the +// strongest argument for shipping `cache` rather than a placement preset. +// +// The provider caps `cache_control` markers at 4. Exceed it and the request is REJECTED — and +// Claude Code's reaction to a rejected capability is to retry without it and leave prompt +// caching OFF for the rest of the conversation. So a breakpoint-budget mistake is not an error +// the user sees; it silently switches off the thing this whole funnel is selling, and the demo +// reads as "context-guru made my session more expensive". +// +// cachesplit MOVES a breakpoint onto the stable half of a block it splits; it must never add +// one. The body below arrives at the cap, so any addition at all is a 400. +func TestCachePresetNeverAddsACacheControlBreakpoint(t *testing.T) { + var up upstreamCapture + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + up.record(r) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"type":"message","usage":{"input_tokens":1}}`) + })) + defer upstream.Close() + + h, _ := buildHandler(t, cachePipeline, upstream.URL) + srv := httptest.NewServer(h.Mux()) + defer srv.Close() + + // Four inbound breakpoints — the provider's cap — spread the way a real client spreads + // them: two system blocks, one tool, one message. Assembled as text, for the key-order + // reason documented on claudeCodeBody. + bp := `,"cache_control":{"type":"ephemeral"}` + body := []byte(`{"model":"claude-sonnet-5","max_tokens":64,"system":[` + + `{"type":"text","text":` + jsonStr(attributionText) + bp + `},` + + `{"type":"text","text":` + jsonStr(volatileSystemText()) + bp + `}` + + `],"tools":[{"name":"read_file","input_schema":{"type":"object"}` + bp + `}` + + `],"messages":[{"role":"user","content":[{"type":"text","text":"hello"` + bp + `}]}]}`) + if !json.Valid(body) { + t.Fatalf("test fixture is not valid JSON: %s", body) + } + + inbound := countBreakpoints(body) + if inbound != 4 { + t.Fatalf("test setup is wrong: the request carries %d breakpoints, not the cap of 4", inbound) + } + resp, err := http.Post(srv.URL+"/anthropic/v1/messages", "application/json", strings.NewReader(string(body))) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + forwarded := up.body(1) + if len(forwarded) == 0 { + t.Fatal("upstream received nothing") + } + // The precondition that stops this being a vacuous pass: the component under test has to + // have ACTED. If cachesplit did not split, "breakpoints unchanged" is trivially true and + // asserts nothing about the rewrite. + if n := len(gjson.GetBytes(forwarded, "system").Array()); n != 3 { + t.Fatalf("cachesplit did not split the volatile tail (system has %d blocks, want 3): "+ + "the breakpoint assertion below would be vacuous", n) + } + if out := countBreakpoints(forwarded); out != inbound { + t.Errorf("breakpoints on the wire = %d, inbound = %d (cap 4). Exceeding the cap is a "+ + "400, and Claude Code answers a rejected cache_control by disabling prompt caching "+ + "for the rest of the conversation — silently switching off what this preset exists "+ + "to demonstrate.\nforwarded: %s", out, inbound, forwarded) + } +} + +// countBreakpoints counts cache_control/cachePoint markers anywhere in the body. Both spellings, +// because Bedrock/Vertex write `cachePoint` where Anthropic writes `cache_control`, and the +// provider's cap counts whatever arrives. +func countBreakpoints(body []byte) int { + n := 0 + var walk func(gjson.Result) + walk = func(v gjson.Result) { + v.ForEach(func(k, val gjson.Result) bool { + if k.String() == "cache_control" || k.String() == "cachePoint" { + n++ + } + if val.IsObject() || val.IsArray() { + walk(val) + } + return true + }) + } + walk(gjson.ParseBytes(body)) + return n +} + +// TestCachePresetLeavesTheAttributionBlockUntouched covers conformance item 3. +// +// Claude Code prepends an attribution block as the FIRST system block, and the API strips it +// only if that array arrives unchanged. cachesplit reshapes the system array, so the question is +// whether the first block survives byte-identically. +// +// Three separate properties keep it safe, and the second is the one a plausible change would +// break, so both are exercised below: +// +// 1. the attribution block carries no volatile marker, so it is not a split candidate; +// 2. blocks the split does not act on are re-emitted from their ORIGINAL raw bytes rather than +// re-encoded — re-marshalling would reorder keys and change the bytes even with identical +// content, which is enough to defeat a positional strip; +// 3. the split's minSplitTokens floor (1024) excludes a small block even when it does contain a +// marker — the second case below, where a user's own prompt happens to mention one. +// +// Proving this rather than reasoning about it is what makes the alternative — shipping +// CLAUDE_CODE_ATTRIBUTION_HEADER=0 in the installer — unnecessary, and keeps it unnecessary. +func TestCachePresetLeavesTheAttributionBlockUntouched(t *testing.T) { + for _, c := range []struct{ name, first string }{ + {"the ordinary attribution block", attributionText}, + // Small, but it mentions something the split looks for. Only the token floor keeps this + // out of the rewrite; without it the FIRST eligible block is the one that gets split, + // and that is this one. + {"a small first block that happens to name a volatile marker", + attributionText + "\nCurrent branch: whatever the user was talking about\n"}, + } { + t.Run(c.name, func(t *testing.T) { + var up upstreamCapture + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + up.record(r) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"type":"message","usage":{"input_tokens":1}}`) + })) + defer upstream.Close() + + h, _ := buildHandler(t, cachePipeline, upstream.URL) + srv := httptest.NewServer(h.Mux()) + defer srv.Close() + + body := claudeCodeBodyWithFirst(t, false, c.first) + want := gjson.GetBytes(body, "system.0").Raw + + resp, err := http.Post(srv.URL+"/anthropic/v1/messages", "application/json", + strings.NewReader(string(body))) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + forwarded := up.body(1) + if len(forwarded) == 0 { + t.Fatal("upstream received nothing") + } + // Precondition: cachesplit must actually have rewritten the array, or "the first + // block is unchanged" is true because nothing happened. It must also have split the + // SECOND block, not the first — 3 blocks with the attribution intact is the only + // shape that means that. + blocks := gjson.GetBytes(forwarded, "system").Array() + if len(blocks) != 3 { + t.Fatalf("cachesplit did not act (system has %d blocks, want 3); the assertion "+ + "below would be vacuous", len(blocks)) + } + if got := blocks[0].Raw; got != want { + t.Errorf("the attribution block changed, so the API will no longer strip it "+ + "positionally.\n got: %s\nwant: %s", got, want) + } + if !strings.HasPrefix(blocks[0].Get("text").String(), attributionText) { + t.Errorf("the attribution block is no longer the first system block: %s", blocks[0].Raw) + } + }) + } +} diff --git a/proxy/counttokens_test.go b/proxy/counttokens_test.go index 14e73518..9e9cd98c 100644 --- a/proxy/counttokens_test.go +++ b/proxy/counttokens_test.go @@ -18,11 +18,9 @@ import ( // unmodified (the client is asking about the context IT holds, and uses the answer to budget its // own transcript), and the answer must come back verbatim. func TestCountTokensIsServed(t *testing.T) { - var gotPath string - var gotBody []byte + var up upstreamCapture upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - gotPath = r.URL.Path - gotBody, _ = io.ReadAll(r.Body) + up.record(r) w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"input_tokens":4321}`) })) @@ -45,6 +43,7 @@ func TestCountTokensIsServed(t *testing.T) { t.Fatalf("status = %d, want 200 (a 404 here sends the client back to counting with "+ "inference requests): %s", resp.StatusCode, out) } + gotPath, gotBody := up.round(1).path, up.body(1) if gotPath != "/v1/messages/count_tokens" { t.Errorf("upstream path = %q, want /v1/messages/count_tokens", gotPath) } diff --git a/proxy/keepalive.go b/proxy/keepalive.go index 50ff32a9..ad4f86cd 100644 --- a/proxy/keepalive.go +++ b/proxy/keepalive.go @@ -1158,6 +1158,52 @@ type KeepAliveStats struct { SpentUSD float64 `json:"spend_usd"` } +// PendingPings reports how many tracked sessions still have a ping scheduled ahead of them. +// +// This exists for the idle-exit watchdog, and it exists because the keep-alive INVERTS the +// ordinary meaning of "idle": pinging is what the proxy does precisely while no client +// traffic is arriving. A watchdog counting only requests would therefore kill the process in +// exactly the window the feature was built for — the quiet gap after `end_turn`, where 83.7% +// of the recoverable dollars sit. So "no requests recently" is not sufficient to exit; "and +// nothing is waiting to be pinged" is the other half. +// +// The conditions are `due`'s minus the timing term: an entry that is stopped, or has spent its +// MaxPings, or whose policy is off will never be pinged again and must not hold the process open. +// +// One caveat, because "a live entry is by construction one we intend to ping" is very nearly true +// and not exactly: `sweep` also drops entries whose `pingable()` has since gone false, and it only +// looks once `now >= startedAt + Idle`. Inside that window this counts an entry that will never +// actually be pinged, which resets the activity clock and delays the exit by up to one `Idle`. +// Bounded by the entry's own retire timer at `(MaxPings+1) * Idle`, and immaterial against a 24h +// threshold — but the honest statement is "outside that window", not "by construction". +// +// Erring toward counting is the right direction anyway: over-counting delays an exit by minutes, +// while under-counting kills the process during the quiet gap the keep-alive exists to work in. +func (h *Handler) PendingPings() int { + if h == nil { + return 0 + } + return h.keeper.pendingPings() +} + +// pendingPings counts entries with a ping still ahead of them. Nil-safe: a keeper whose +// sweep never launched (the CONTEXT_GURU_KEEPALIVE kill switch) has nothing pending, which +// correctly lets an idle proxy exit. +func (k *keeper) pendingPings() int { + if k == nil { + return 0 + } + k.mu.Lock() + defer k.mu.Unlock() + n := 0 + for _, e := range k.live { + if !e.stopped && e.pol.on() && e.pings < e.pol.MaxPings { + n++ + } + } + return n +} + // Stats snapshots the keeper's counters. func (k *keeper) Stats() KeepAliveStats { if k == nil { diff --git a/store/idleexit_test.go b/store/idleexit_test.go new file mode 100644 index 00000000..0a0bd447 --- /dev/null +++ b/store/idleexit_test.go @@ -0,0 +1,125 @@ +package store + +import ( + "strings" + "testing" + "time" +) + +// TestIdleExitFloorRefusesADestructiveThreshold is about money, not tidiness. +// +// Process exit wipes this store, and what it wipes includes frozen decisions. A frozen +// decision that dies mid-session is the 11.5x cache-WRITE regression FrozenLost exists to +// detect: the next turn re-creates the whole prefix at write prices instead of reading it. So a +// short idle-exit threshold does not degrade gracefully — it turns a convenience feature into a +// cost regression that presents as the proxy misbehaving, on the machine of the first-time +// evaluator this whole funnel is aimed at. +// +// Hence a startup error rather than a doc comment. The 30-minute case below is the one somebody +// will actually reach for ("exit quickly, it is only a laptop"), and it must not start. +func TestIdleExitFloorRefusesADestructiveThreshold(t *testing.T) { + def := Options{} // ttl_seconds unset => DefaultTTL (10000s), floor 2x = 5h33m20s + if got, want := IdleExitFloor(def), 2*DefaultTTL; got != want { + t.Fatalf("IdleExitFloor(default) = %s, want %s", got, want) + } + + for _, c := range []struct { + name string + d time.Duration + o Options + wantErr bool + }{ + {"off is always valid", 0, def, false}, + {"negative is off too", -time.Hour, def, false}, + {"30m on the default TTL is destructive", 30 * time.Minute, def, true}, + {"1h is still below the default floor", time.Hour, def, true}, + {"just under the floor", 2*DefaultTTL - time.Second, def, true}, + {"exactly the floor is allowed", 2 * DefaultTTL, def, false}, + {"the installer's 24h default", 24 * time.Hour, def, false}, + // A tiny configured TTL must not collapse the floor to seconds: 2x30s is 1m, which is + // shorter than the keep-alive's own ping window, so the absolute 1h term takes over. + {"tiny ttl falls back to the 1h term", 30 * time.Minute, Options{TTLSeconds: 30}, true}, + {"tiny ttl accepts 1h", time.Hour, Options{TTLSeconds: 30}, false}, + // A LONG configured TTL must raise the floor above 1h, or an operator who deliberately + // widened the store's lifetime gets a threshold that expires it. + {"long ttl raises the floor above 24h", 24 * time.Hour, Options{TTLSeconds: 100000}, true}, + } { + err := ValidateIdleExit(c.d, c.o) + if c.wantErr && err == nil { + t.Errorf("%s: ValidateIdleExit(%s, ttl=%s) accepted a threshold below the %s floor", + c.name, c.d, c.o.EffectiveTTL(), IdleExitFloor(c.o)) + continue + } + if !c.wantErr && err != nil { + t.Errorf("%s: ValidateIdleExit(%s, ttl=%s) rejected a valid threshold: %v", + c.name, c.d, c.o.EffectiveTTL(), err) + continue + } + // The message has to tell the operator what to change. A bare "invalid value" here + // leaves them guessing at a number they have no reason to know. + if err != nil { + for _, want := range []string{"idle-exit", "floor"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("%s: error message omits %q: %v", c.name, want, err) + } + } + } + } +} + +// TestEffectiveTTLIsWhatNewMemoryUses closes the gap the floor depends on: IdleExitFloor sizes +// a process's whole lifetime from EffectiveTTL, so a store that actually ran with a DIFFERENT +// lifetime would be protected by a floor computed for a lifetime it never had. NewMemory calls +// EffectiveTTL rather than repeating the defaulting rule, and this holds it there. +func TestEffectiveTTLIsWhatNewMemoryUses(t *testing.T) { + for _, o := range []Options{{}, {TTLSeconds: 0}, {TTLSeconds: -5}, {TTLSeconds: 42}, {TTLSeconds: 100000}} { + if got := NewMemory(o).ttl; got != o.EffectiveTTL() { + t.Errorf("NewMemory(%+v).ttl = %s but EffectiveTTL() = %s; the idle-exit floor is "+ + "computed from the second and would protect a lifetime the store is not using", + o, got, o.EffectiveTTL()) + } + } +} + +// TestValidateIdleExitMessageNamesTheBindingTerm: this refusal is startup-fatal, so its message is +// the only evidence anyone gathers — and it was wrong in both halves. +// +// It credited the floor to "2x the store's entry lifetime" even when the absolute 1h term was the +// binding one (with ttl_seconds: 30 it announced a 1h floor derived from 2x30s, which is 1m), and it +// advised raising ttl_seconds — the one change that RAISES the floor, so an operator who followed it +// got the same refusal with a bigger number. +func TestValidateIdleExitMessageNamesTheBindingTerm(t *testing.T) { + // (a) the store term binds: default TTL of 10000s puts the floor at 5h33m20s. + err := ValidateIdleExit(30*time.Minute, Options{}) + if err == nil { + t.Fatal("30m accepted against the default floor") + } + msg := err.Error() + if !strings.Contains(msg, "2x the store's") { + t.Errorf("the store term binds but the message does not say so: %v", err) + } + if !strings.Contains(msg, "LOWER store.ttl_seconds") { + t.Errorf("message does not offer the remediation that actually lowers the floor: %v", err) + } + if strings.Contains(msg, "raise store.ttl_seconds") { + t.Errorf("message still advises RAISING ttl_seconds, which raises the floor: %v", err) + } + + // (b) the absolute term binds: a 30s TTL makes 2x TTL 1m, so the 1h minimum is doing the work + // and ttl_seconds is not the lever. + err = ValidateIdleExit(30*time.Minute, Options{TTLSeconds: 30}) + if err == nil { + t.Fatal("30m accepted against the 1h minimum") + } + msg = err.Error() + if !strings.Contains(msg, "absolute minimum") { + t.Errorf("the 1h term binds but the message does not say so: %v", err) + } + if strings.Contains(msg, "which is 2x the store's") { + t.Errorf("message attributes a 1h floor to 2x a 30s lifetime, which is 1m: %v", err) + } + // It should still tell the operator what 2x TTL actually is, so the arithmetic is checkable. + if !strings.Contains(msg, "1m0s") { + t.Errorf("message does not show what the 2x-TTL floor would be (1m0s): %v", err) + } +} diff --git a/store/store.go b/store/store.go index cd6bc1f7..8f224fb5 100644 --- a/store/store.go +++ b/store/store.go @@ -13,6 +13,7 @@ package store import ( "container/list" + "fmt" "strings" "sync" "time" @@ -299,13 +300,83 @@ const DefaultMaxEntries = 5000 // (stash_refused), instead of quietly making them irreversible. const DefaultStashMaxBytes = 256 << 20 +// EffectiveTTL is the entry lifetime this Options actually yields, defaulting included. +// +// Exported and used by NewMemory itself rather than duplicated, because a second copy of +// "zero means DefaultTTL" is exactly the drift that would matter: IdleExitFloor sizes a +// process's whole lifetime off this number, and a floor computed from a different default +// than the store runs with is a floor that protects nothing. +func (o Options) EffectiveTTL() time.Duration { + if o.TTLSeconds <= 0 { + return DefaultTTL + } + return time.Duration(o.TTLSeconds) * time.Second +} + +// IdleExitFloor is the shortest idle-exit threshold that is not destructive. +// +// Process exit WIPES this store: rewind stashes, frozen decisions, `cg:len:`. A frozen +// decision that dies mid-session is the 11.5x cache-WRITE regression that FrozenLost exists +// to detect — the session's next turn re-creates the whole prefix at write prices instead of +// reading it. So an idle-exit threshold shorter than the store's own entry lifetime turns a +// convenience feature into a cost regression that looks like the proxy misbehaving. +// +// 2x the TTL, with a 1h absolute floor. Twice, not once, because the TTL is a SLIDING +// window: an entry touched just before the idle clock started still has a full TTL ahead of +// it, so 1x can expire live state. The 1h term covers a config that sets a tiny ttl_seconds +// (a test rig, or an operator trimming memory) where 2x would collapse to seconds and the +// threshold would be shorter than the keep-alive's own ping window. +// +// With the default TTL of 10000s the floor is ~5h34m, so the installer's 24h default clears +// it comfortably; a 30-minute threshold is refused at startup rather than documented. +func IdleExitFloor(o Options) time.Duration { + if f := 2 * o.EffectiveTTL(); f > time.Hour { + return f + } + return time.Hour +} + +// ValidateIdleExit checks an idle-exit threshold against IdleExitFloor. Zero or negative +// means the watchdog is off, which is always valid — a gateway or eval-containers +// deployment must never self-terminate, so off is the default and the only way to a +// self-killing proxy is to ask for one. +// +// The message says WHICH of the floor's two terms produced the number, and offers only remediations +// that actually lower it. An earlier version did neither: it credited the floor to "2x the store's +// entry lifetime" even when the 1h term was the binding one (with ttl_seconds: 30 it printed a 1h +// floor attributed to 2x30s), and it advised raising ttl_seconds — which raises the floor, so an +// operator who followed it got the same refusal with a larger number. Startup-fatal messages are the +// only evidence anyone gathers, so being exactly right here matters more than it looks. +func ValidateIdleExit(d time.Duration, o Options) error { + if d <= 0 { + return nil + } + floor := IdleExitFloor(o) + if d >= floor { + return nil + } + ttl := o.EffectiveTTL() + if 2*ttl > time.Hour { + // The store term binds: lowering the TTL lowers the floor with it. + return fmt.Errorf("idle-exit %s is below the floor of %s, which is 2x the store's %s entry "+ + "lifetime: exiting wipes the in-memory store, so a shorter threshold drops live frozen "+ + "decisions and re-bills their prefix as cache creation instead of a cache read. Raise "+ + "--idle-exit to at least %s, or LOWER store.ttl_seconds if the short threshold is what "+ + "you want (the floor follows it)", d, floor, ttl, floor) + } + // The absolute term binds: ttl_seconds is not what is stopping this, so do not mention it as a + // lever. 2x this TTL is only %s, which is why the 1h minimum is doing the work. + return fmt.Errorf("idle-exit %s is below the absolute minimum of %s. The store's entry lifetime "+ + "is %s, so the 2x-TTL floor would only be %s and is not what refuses this: a threshold "+ + "under an hour is shorter than the keep-alive's own ping window, and exiting inside it "+ + "drops live frozen decisions. Raise --idle-exit to at least %s", + d, floor, ttl, 2*ttl, floor) +} + // NewMemory builds an in-memory store. Zero/negative option fields fall back to // defaults (DefaultTTL, DefaultMaxEntries, 100 sessions of sticky sets). func NewMemory(o Options) *Memory { - ttl := time.Duration(o.TTLSeconds) * time.Second - if o.TTLSeconds <= 0 { - ttl = DefaultTTL - } + ttl := o.EffectiveTTL() max := o.MaxEntries if max <= 0 { max = DefaultMaxEntries