From 0ad717ea83365b361fa2ac5841bcc32efce8652b Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Tue, 1 Sep 2026 11:09:24 +0300 Subject: [PATCH 1/6] feat(dist): pure-Go releases, a `cache` preset, --idle-exit, and gateway conformance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the local-distribution proposal (#130) minus the Claude Code plugin, which ships as its own PR because all six blocking findings from the review of #141 live in it. Nothing here is held behind that. `docs/get-started/quickstart-proxy.md`, `docs/setup.md` and `docs/hosted.md` all told evaluators to install a C toolchain and set `CGO_ENABLED=1`. That is true only for a `cg_skeleton` build. setup.md went further and named bifrost's tokenizer as a cgo dependency, which it is not — o200k_base is embedded (`internal/tokens/tokens.go`). Verified directly on go 1.26.4 rather than taken from the proposal: `CGO_ENABLED=0` with default tags builds all four release targets (linux/darwin x amd64/arm64, 27.1-33.5 MB stripped), `file` reports "statically linked", `ldd` reports "not a dynamic executable", the binary starts and answers /healthz, and `-tags cg_skeleton` fails under CGO_ENABLED=0 with the build-constraints signature — confirming tree-sitter is the only C dependency. - `.goreleaser.yaml`: a plain GOOS/GOARCH matrix, no cross-toolchains, no `brews:` block (the tap repo and release signing are an unowned question, and nothing may depend on a repo that does not exist). - `.github/workflows/release.yaml`: a tag publishes, `workflow_dispatch` builds the same matrix as a snapshot. It asserts the pure-Go claim with `CC=/nonexistent-c-compiler`. - `make build-static`. The Makefile keeps `CGO_ENABLED=1` because `go test -race` needs it, and the comment now says that is a test-time requirement — reading it as a shipping requirement is how the wrong claim reached the docs. The funnel's default, chosen so a stranger can verify the claim by reading one line rather than trusting four components. Not `safe`, whose extra components are lossless in meaning but still rewrite the JSON. Off by default; a gateway or eval-containers deployment must never self-terminate. A signal and the watchdog converge on the SAME teardown, so the self-killing path cannot drift from the one known to work. Two properties are load-bearing: - **The keep-alive inverts "idle."** Pinging is what the proxy does precisely while no client traffic arrives — the quiet gap after `end_turn`, where 83.7% of the recoverable dollars sit. A pending ping both vetoes the exit and RESETS the clock, so retiring the last ping buys a full fresh threshold rather than exiting moments later. - **Exit wipes the in-memory store.** `store.ValidateIdleExit` refuses anything below `max(2 x store.ttl_seconds, 1h)` at startup — ~5h34m at the default. 2x because the TTL is a sliding window. `NewMemory` now calls the same `Options.EffectiveTTL` the floor is computed from, so the two cannot drift. All five items from the proposal, under the `cache` preset. Four were already correct and are now pinned by tests; `POST /anthropic/v1/messages/count_tokens` was missing entirely — without it a client counts context by issuing INFERENCE requests, billed calls added by a proxy sold on removing them. Five places promised it did not: `config/config.go`, `docs/reference/presets.md`, `docs/how-to/choose-a-preset.md`, the plugin doc, and the install skill. Verified before fixing — `[Read Bash]` in, `[Read Bash context_guru_expand]` out on the real gateway route. Root cause was a code-vs-comment contradiction. `Options.InjectExpand` documented the gate as requiring "an expandable marker"; `expand/inject.go` says "No marker condition, deliberately" and the real conditions were mode, store-persists and has-tools. Nothing asked whether the pipeline could produce a marker at all. `components.Pipeline.HasOffload()` answers that by type assertion rather than a name list (a name list is a second copy of "which components are lossy" and drifts the moment somebody adds one). Under `auto`, injection now requires it. `always` still injects unconditionally — an operator who asks for it by name gets it. This also fixes `off`, the A/B control arm, which was carrying an extra tool declaration. Marker independence is preserved, which is the invariant that matters for cache stability: a pipeline does not change turn to turn, so the tools array stays byte-stable across a session. **Ten existing expand tests changed fixture, and that is worth reading.** They hand-seed the Store to simulate an offload, but built their handler with `pipeline: []` — a pipeline that cannot offload anything. That was harmless only while injection ignored the pipeline. They now use `offloadCapablePipeline` (`[linecap]`, which does not act on their short bodies), so each fixture matches its own premise. No assertion was weakened. - **`--idle-exit` was defeated by any health probe** (finding 7). `/healthz` and `/metrics` no longer count as activity: a probe on a schedule shorter than the threshold meant the exit NEVER fired and logged nothing to say so — measured, a 1h-threshold proxy reporting "idle for 1h3m0s" after 2h03m. A dashboard poll still counts, deliberately: a probe is not a viewer, and exiting under somebody who is watching is the worse failure. - **A gateway may no longer self-terminate.** `--idle-exit` with `--upstreams` is refused at startup. That safety was previously accidental — it held only because hosted deployments run a liveness probe, which the change above stops counting. - **The floor's refusal was logged after "listening"**, so a rejected configuration read as a crash. Both refusals moved earlier and into one testable `checkIdleExit`. - **`--listen` and `--version` flags** (findings 2 and 12, which are the plugin's, but the flags are the core's). The address reached the process only through the environment, so no supervisor or `ps` could tell which port an instance held; and `buildinfo.Version` was reachable only via `/stats` on a running proxy, so an installer asking `--help` recorded "Usage of context-guru-proxy:" as the installed version. - **Nothing tested the shipped configuration** (finding 9). A tag published without running any tests, and CI runs the suite only with `CGO_ENABLED=1` — so `TestEveryPresetBuilds`, which guards exactly the CGO-free artifact, was never executed in that configuration. The release workflow now runs a CGO-off suite over the packages whose behaviour depends on which components are compiled in, plus the full suite, before publishing. It also asserts `--version` answers. - **`scripts/gate-a-purego.sh` was cited as proof in four places and is not in this PR** (finding 8). Those now cite the release workflow's own assert step, which exists here and fails the release if a cgo dependency escapes the `cg_skeleton` tag. - **The savings claim was measured in the wrong regime.** −34.1% / 96.7% comes from a harness running tasks back-to-back inside the provider's 5-minute TTL, and is one task measured three times; this project's own interactive figure is $0.0298 across 1,127 sessions, with 1,105 of 1,127 session starts reading zero from cache. Both are now stated, with the zero cases (outside a git repo, under the 1,024-token floor, non-Anthropic backend). The old citation pointed at `docs/results/context-guru.md`, which contains neither number. - **`count_tokens` behaviour kept, consequence documented** (finding 10). It answers about the ORIGINAL body — over-reporting is recoverable, under-reporting costs a failed turn when a fail-open component reverts. What was undocumented is the cost: the client self-compacts earlier than needed (115,933 reported vs 32,802 forwarded). Now in `docs/reference/routes.md`, where the route was absent entirely. Its **hosted branch was wholly untested** — that branch is all that stands between the multi-tenant service and an unmetered open forwarder — so it now has one. - `--idle-exit` and `--version` added to both flag tables; `bytes.NewReader` in counttokens. Six mutations, each proven to have landed in the source before its result was allowed to count: expand injection ungated (the defect) -> TestCachePresetAdvertisesNoExtraTool FAIL cache: sent [Read Bash], forwarded [Read Bash context_guru_expand] off: sent [Read Bash], forwarded [Read Bash context_guru_expand] HasOffload always true -> same test FAIL, same two subcases HasOffload always false -> FAIL on the offloader subcase: "mints markers but no longer advertises the expand tool, so a model cannot recover what it offloaded" probes count as activity again -> TestProbesDoNotDeferIdleExit FAIL ("two hours of nothing but liveness probes: idle past the threshold, but watchIdle never exited") gateway guard disabled -> TestCheckIdleExitRefusesAGatewaySelfTerminating FAIL count_tokens hosted auth removed -> TestCountTokensHostedRequiresAuth FAIL (502, want 401) The third mutation is the one worth noting: it proves the fix did not trade a silent defect for its mirror image, an offloader whose output nothing can expand. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- .github/workflows/release.yaml | 90 +++++++ .goreleaser.yaml | 98 +++++++ README.md | 5 + cmd/context-guru-proxy/idleexit.go | 172 +++++++++++++ cmd/context-guru-proxy/idleexit_test.go | 329 ++++++++++++++++++++++++ cmd/context-guru-proxy/main.go | 79 +++++- config/config.go | 22 +- config/config_more_test.go | 41 ++- docs/get-started/quickstart-proxy.md | 19 +- docs/how-to/choose-a-preset.md | 35 +++ docs/how-to/use-with-claude-code.md | 19 ++ docs/reference/config.md | 4 +- docs/reference/presets.md | 1 + docs/setup.md | 10 +- proxy/conformance_test.go | 305 ++++++++++++++++++++++ proxy/keepalive.go | 38 +++ store/idleexit_test.go | 82 ++++++ store/store.go | 60 ++++- 18 files changed, 1391 insertions(+), 18 deletions(-) create mode 100644 .github/workflows/release.yaml create mode 100644 .goreleaser.yaml create mode 100644 cmd/context-guru-proxy/idleexit.go create mode 100644 cmd/context-guru-proxy/idleexit_test.go create mode 100644 proxy/conformance_test.go create mode 100644 store/idleexit_test.go diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml new file mode 100644 index 00000000..2908ad54 --- /dev/null +++ b/.github/workflows/release.yaml @@ -0,0 +1,90 @@ +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: + go-version: '1.26' + check-latest: true + + # 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" + run: go test ./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..8b568a1a --- /dev/null +++ b/.goreleaser.yaml @@ -0,0 +1,98 @@ +# 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] + files: + - LICENSE + - README.md + - THIRD-PARTY-NOTICES + +checksums: + # scripts/install.sh verifies the downloaded tarball against this file. macOS quarantines + # an unsigned download and the installer strips the attribute, so a checksum is the only + # integrity check left in that path — it is not optional decoration. + 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`: + + ``` + tar xzf context-guru_*_darwin_arm64.tar.gz + install -m 755 context-guru-proxy ~/.local/bin/ + ``` + + Then see `docs/get-started/quickstart-proxy.md`. diff --git a/README.md b/README.md index 3fd159ec..2d1db4fc 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,6 +147,8 @@ 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 | | `--anthropic-upstream` / `ANTHROPIC_UPSTREAM` | `https://api.anthropic.com` | Anthropic upstream base | diff --git a/cmd/context-guru-proxy/idleexit.go b/cmd/context-guru-proxy/idleexit.go new file mode 100644 index 00000000..023afc13 --- /dev/null +++ b/cmd/context-guru-proxy/idleexit.go @@ -0,0 +1,172 @@ +package main + +import ( + "fmt" + "net/http" + "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". +// Nanoseconds in an atomic so the request path pays one store and no lock. +type activityClock struct{ ns atomic.Int64 } + +func (a *activityClock) touch(now time.Time) { a.ns.Store(now.UnixNano()) } +func (a *activityClock) last() time.Time { return time.Unix(0, a.ns.Load()) } + +// probeRoutes are the 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, and the session hook's +// own start-up check. Counting them was a bug that disabled the whole feature rather than +// weakening it — measured: a proxy with a 1h threshold logged +// `idle-exit armed after=1h0m0s`, 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 logs nothing to say so. +// +// 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 is a deliberate asymmetry — a probe is not a viewer. +var probeRoutes = map[string]bool{ + "/healthz": true, + "/metrics": true, +} + +// stampActivity records a request as activity, unless its route is a machine probe. +// +// The stamp happens BEFORE the handler runs, so a long streaming response cannot age out while +// it is still being served — its own duration is not idleness. (The stream also cannot be cut +// off mid-flight regardless: srv.Shutdown waits for in-flight requests, which is why this +// reuses that path rather than calling os.Exit.) +func stampActivity(next http.Handler, act *activityClock, now func() time.Time) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !probeRoutes[r.URL.Path] { + act.touch(now()) + } + next.ServeHTTP(w, r) + }) +} + +// 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. + if o.act.last().UnixNano() == 0 { + 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 keeps the +// exit within 5% of what was asked for, clamped so a 24h default does not mean an hour of +// slack and a 1h floor does not mean a check every three minutes. +func idleCheckInterval(threshold time.Duration) time.Duration { + d := threshold / 20 + if d < 30*time.Second { + d = 30 * time.Second + } + if d > 5*time.Minute { + d = 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 string, o store.Options) error { + // The floor. Exiting clears the in-memory store, and losing a live frozen decision re-bills + // its whole prefix as cache creation — the 11.5x regression FrozenLost exists to catch. + if err := store.ValidateIdleExit(d, o); err != nil { + return err + } + if d > 0 && upstreamsPath != "" { + // 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. + return fmt.Errorf("--idle-exit cannot be combined with --upstreams: a gateway serving " + + "other people's agents must not self-terminate. Drop --idle-exit, or run this " + + "instance without --upstreams") + } + 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..9ecd36f0 --- /dev/null +++ b/cmd/context-guru-proxy/idleexit_test.go @@ -0,0 +1,329 @@ +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 + h := stampActivity(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // The stamp must land BEFORE the handler runs, so a long streaming response cannot + // age out while it is still being served. + stampedBeforeHandler = act.last().Equal(clk.now()) + w.WriteHeader(http.StatusOK) + }), 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 the resolution at both ends: a 24h default must not +// mean an hour of slack past the threshold, and the 1h floor must not mean a check every few +// minutes for nothing. +func TestIdleCheckIntervalStaysUseful(t *testing.T) { + for _, c := range []struct{ threshold, want time.Duration }{ + {24 * time.Hour, 5 * time.Minute}, // clamped high + {time.Hour, 3 * time.Minute}, // threshold/20 + {10 * time.Minute, 30 * time.Second}, // clamped low + } { + if got := idleCheckInterval(c.threshold); got != c.want { + t.Errorf("idleCheckInterval(%s) = %s, want %s", c.threshold, got, c.want) + } + } +} + +// 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(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }), 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(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), 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) + } + } +} diff --git a/cmd/context-guru-proxy/main.go b/cmd/context-guru-proxy/main.go index f2eeeaa0..d880748d 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,14 @@ 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. + // Set only by the plugin installer, which pairs it with a SessionStart hook that + // starts the proxy again on demand — self-kill without that resurrection is a + // footgun, so they ship together. The floor is enforced below, 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,6 +204,17 @@ 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, @@ -571,6 +596,17 @@ func main() { slog.Warn("context-guru: OBSERVE MODE — requests are forwarded UNMODIFIED; " + "/stats reports what compaction WOULD have saved under potential_*/projected_* keys") } + // Idle-exit validation goes BEFORE the "listening" line, because a fatal here used to be + // logged after it: the operator saw `context-guru-proxy listening` and then an exit, which + // reads as a crash rather than as a rejected configuration. + // + // Refused rather than warned about: a threshold below the store's entry lifetime does not + // degrade gracefully, it re-bills live prefixes as cache creation (the 11.5x regression + // FrozenLost exists to catch). A misconfigured value must not start. + if err := checkIdleExit(*idleExit, *upstreamsPath, cfg.Store); err != nil { + log.Fatalf("context-guru: %v", err) + } + // The sink last, so it is the line just above the traffic: "where are the logs and // what level am I getting" is the first question when something looks quiet. ln, err := listenAndAnnounce(addr, "pipeline", cfg.Pipeline, "mode", mode, "logs", sink) @@ -578,9 +614,22 @@ 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. + var handler http.Handler = h.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(handler, 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 +649,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 { 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..c554a9ad 100644 --- a/docs/get-started/quickstart-proxy.md +++ b/docs/get-started/quickstart-proxy.md @@ -3,19 +3,30 @@ 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 +tar xzf context-guru_*_darwin_arm64.tar.gz +install -m 755 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..68862888 100644 --- a/docs/how-to/use-with-claude-code.md +++ b/docs/how-to/use-with-claude-code.md @@ -3,6 +3,22 @@ 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. + ## Steps 1. Start the proxy: @@ -38,6 +54,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..c2369bba --- /dev/null +++ b/proxy/conformance_test.go @@ -0,0 +1,305 @@ +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 forwarded []byte + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + forwarded, _ = io.ReadAll(r.Body) + 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() + 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 forwarded []byte + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + forwarded, _ = io.ReadAll(r.Body) + 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() + 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/keepalive.go b/proxy/keepalive.go index 50ff32a9..bf6d39ca 100644 --- a/proxy/keepalive.go +++ b/proxy/keepalive.go @@ -1158,6 +1158,44 @@ 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. Everything else is gated at record time (see pingable), so a live entry is by +// construction one we intend to ping. +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..f200a9e1 --- /dev/null +++ b/store/idleexit_test.go @@ -0,0 +1,82 @@ +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()) + } + } +} diff --git a/store/store.go b/store/store.go index cd6bc1f7..8132644f 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,64 @@ 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. +func ValidateIdleExit(d time.Duration, o Options) error { + if d <= 0 { + return nil + } + if floor := IdleExitFloor(o); d < floor { + return fmt.Errorf("idle-exit %s is below the floor of %s (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, or raise store.ttl_seconds if the short lifetime is "+ + "deliberate", d, floor, o.EffectiveTTL()) + } + return nil +} + // 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 From b652546eed4507a87da1ed37bfb59fed43e0be98 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Wed, 2 Sep 2026 09:02:48 +0300 Subject: [PATCH 2/6] ci: read the Go toolchain from go.mod everywhere (#152) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI validated on Go 1.25 while `go.mod` declared 1.26.4 and this PR's release workflow built on 1.26 — three numbers that have to agree, with nothing making them. That is a distribution bug, not a housekeeping one. `purego` asserts that the SHIPPED artifact builds with cgo off, and `release.yaml` builds the artifact people download; when those run different toolchains from each other and from the module, the assertion describes a build nobody ships. The `purego` job inherited the wrong pin from the job it was copied from, which is exactly how the drift spread in the first place. Fixed as a class rather than an instance: `go-version-file: go.mod` in all three places, so the module file is the single source of truth and the next toolchain bump moves CI, the release build and the module together or not at all. Typing `1.26` in three files would have fixed today's symptom and left tomorrow's. `check-latest` is dropped with the literals — it existed to pick up patch releases of a pinned minor, and go.mod names an exact version. Both workflows re-validated as YAML. The change can only really be proven by CI itself, which is where the previous mismatch was invisible. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- .github/workflows/ci.yaml | 12 ++++++++---- .github/workflows/release.yaml | 5 +++-- 2 files changed, 11 insertions(+), 6 deletions(-) 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 index 2908ad54..e93fc89e 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -24,8 +24,9 @@ jobs: fetch-depth: 0 - uses: actions/setup-go@v5 with: - go-version: '1.26' - check-latest: true + # 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 From e1ef6b9d3cb8a01c08b0252338b8eab822de3e76 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Thu, 3 Sep 2026 13:26:43 +0300 Subject: [PATCH 3/6] =?UTF-8?q?fix(dist):=20act=20on=20the=20#141=20review?= =?UTF-8?q?=20=E2=80=94=20monotonic=20idle=20clock,=20earlier=20refusal,?= =?UTF-8?q?=20honest=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten findings, all addressed. The pattern across most of them is the one this branch keeps rediscovering: a comment that describes something the code does not do. ## Behaviour **The idle clock lost its monotonic reading.** It stored `now.UnixNano()` and rebuilt the instant with `time.Unix(0, ns)`, which carries no monotonic reading — so `now.Sub(act.last())` was wall-clock arithmetic. A laptop suspend/resume or an NTP step counted as idleness, and the watchdog could 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. Now stores the `time.Time` itself. Fixing it broke the unstamped-clock backstop, and the existing test caught that: `time.Time{}`'s `UnixNano()` is a large NEGATIVE number, not zero, so the guard stopped firing and the watchdog exited immediately reporting "idle for 2562047h47m16s". The predicate is `IsZero()` now. **The refusal ran too late to be free.** `checkIdleExit` sat after the dashboard and control SQLite files are opened, and `log.Fatalf` calls `os.Exit`, which runs no defers — so `--idle-exit 30m` created and migrated both databases and then exited with WAL/-shm left behind. Moved to immediately after the config resolves, where everything it reads is already known. **The floor protected a store that need not exist.** `--store=false` resolves to `store.Nop`, which holds no frozen decisions, yet a short threshold was refused with a message about dropping them. Skipped when `Enabled` is explicitly false; `nil` (unconfigured) still means on. **`IDLE_EXIT=86400` silently meant "never".** `envDuration` discarded the parse error, and the `idle-exit armed` line is only logged above zero — so the operator's evidence was the ABSENCE of a line. A non-empty unparseable duration is now fatal, for every caller: each one is a timeout, a retention window or a process lifetime, and a typo in any of them changes behaviour nobody chose. **Requests are stamped on completion as well as entry.** Entry-only meant a long request looked like a gap in use the moment it finished. **A clamp that could never fire is gone.** `checkIdleExit` refuses anything under an hour, so `threshold/20` is always at least three minutes and the `< 30s` branch was unreachable — while the comment above it claimed the clamps prevented "a 1h floor meaning a check every three minutes", which is exactly what an hour yields. ## Comments that were not true - `stampActivity` claimed a long streaming response "cannot age out while it is still being served". It can: the clock is not refreshed DURING a request, so a lone SSE consumer past the threshold is severed by the shutdown `armShutdown` performs. Now states the residual and why it is not worth machinery (the dashboard polls every 30s, and the floor is an hour). - The `--idle-exit` flag comment said it and the resurrection hook "ship together". They do not: the hook is in the plugin PR. Anyone setting this by hand today gets a proxy that exits and stays exited, and the comment says so. - `.goreleaser.yaml` cited `scripts/install.sh` as the consumer of `checksums.txt`. That installer ships with the plugin; today the file is what a human curling a release should check by hand. ## Tests `recordedRequest` (mutex-guarded) replaces three unsynchronised captures the reviewer flagged in `conformance_test.go` and `counttokens_test.go`: a handler goroutine wrote them, the test goroutine read them, and an HTTP round trip is not a happens-before edge the memory model guarantees. Worth stating precisely: `-race` is clean before AND after, so this is a fix by INSPECTION, not one the detector demonstrated. A parallel sweep of the whole suite (28 more instances, its own PR) built a control proving the detector was live and that this shape is invisible to it — an in-process loopback round trip manufactures an edge through net/http's internals that the spec does not promise and a Go release can remove. The `10m -> 30s` interval case was labelled "clamped low" and asserted exactly `10m/20`, so it passed whether or not the clamp existed. Replaced with cases that pin the rule and the cap. New tests, each revert-verified against the pre-fix file with the mutation proven to have landed: Unix-nanos clock -> TestActivityClockKeepsItsMonotonicReading FAIL "the stored instant has no monotonic reading, so idleness is measured against the wall clock" entry-only stamp -> TestStampActivityRefreshesOnCompletion FAIL clock reads the moment the request STARTED, 20 minutes behind its completion floor always enforced -> TestCheckIdleExitSkipsTheFloorWithNoStore FAIL refused 30m with the store disabled, citing frozen decisions that cannot exist silent duration parse -> TestParseEnvDurationRefusesAUnitlessValue FAIL parseEnvDuration("86400") returned the default and no error `parseEnvDuration` is split out of `envDuration` so the decision is testable without a process that calls `os.Exit`. Two findings are verified by inspection only, and neither is testable without a subprocess harness: the refusal's new POSITION (it precedes every `Close`-deferring construction in main) and the three comment corrections. `go build ./...`, `go vet ./...`, `gofmt -l`, the full `go test ./...`, and `go test -race` over ./proxy/ and ./cmd/context-guru-proxy/ are all clean. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- .goreleaser.yaml | 11 +- cmd/context-guru-proxy/idleexit.go | 94 ++++++++++---- cmd/context-guru-proxy/idleexit_test.go | 158 ++++++++++++++++++++++-- cmd/context-guru-proxy/main.go | 64 +++++++--- proxy/ccbody_test.go | 38 ++++++ proxy/conformance_test.go | 10 +- proxy/counttokens_test.go | 7 +- 7 files changed, 322 insertions(+), 60 deletions(-) diff --git a/.goreleaser.yaml b/.goreleaser.yaml index 8b568a1a..959de34a 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -61,9 +61,14 @@ archives: - THIRD-PARTY-NOTICES checksums: - # scripts/install.sh verifies the downloaded tarball against this file. macOS quarantines - # an unsigned download and the installer strips the attribute, so a checksum is the only - # integrity check left in that path — it is not optional decoration. + # 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 diff --git a/cmd/context-guru-proxy/idleexit.go b/cmd/context-guru-proxy/idleexit.go index 023afc13..b5d96046 100644 --- a/cmd/context-guru-proxy/idleexit.go +++ b/cmd/context-guru-proxy/idleexit.go @@ -33,11 +33,28 @@ import ( // refused at startup, not documented — see store.ValidateIdleExit. // activityClock is the last moment this process did something a user would call "in use". -// Nanoseconds in an atomic so the request path pays one store and no lock. -type activityClock struct{ ns atomic.Int64 } +// +// 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) } -func (a *activityClock) touch(now time.Time) { a.ns.Store(now.UnixNano()) } -func (a *activityClock) last() time.Time { return time.Unix(0, a.ns.Load()) } +// 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 paths that do NOT count as use. // @@ -59,16 +76,27 @@ var probeRoutes = map[string]bool{ // stampActivity records a request as activity, unless its route is a machine probe. // -// The stamp happens BEFORE the handler runs, so a long streaming response cannot age out while -// it is still being served — its own duration is not idleness. (The stream also cannot be cut -// off mid-flight regardless: srv.Shutdown waits for in-flight requests, which is why this -// reuses that path rather than calling os.Exit.) +// 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(next http.Handler, act *activityClock, now func() time.Time) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if !probeRoutes[r.URL.Path] { + probe := probeRoutes[r.URL.Path] + if !probe { act.touch(now()) } next.ServeHTTP(w, r) + if !probe { + act.touch(now()) + } }) } @@ -107,7 +135,12 @@ func watchIdle(o idleExitOptions) (string, bool) { // 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. - if o.act.last().UnixNano() == 0 { + // 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 { @@ -130,18 +163,20 @@ func watchIdle(o idleExitOptions) (string, bool) { } } -// idleCheckInterval is how often the watchdog looks. A twentieth of the threshold keeps the -// exit within 5% of what was asked for, clamped so a 24h default does not mean an hour of -// slack and a 1h floor does not mean a check every three minutes. +// 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 - if d < 30*time.Second { - d = 30 * time.Second + if d := threshold / 20; d < 5*time.Minute { + return d } - if d > 5*time.Minute { - d = 5 * time.Minute - } - return d + return 5 * time.Minute } // checkIdleExit is every reason a requested idle-exit threshold must not start. @@ -150,10 +185,21 @@ func idleCheckInterval(threshold time.Duration) time.Duration { // 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 string, o store.Options) error { - // The floor. Exiting clears the in-memory store, and losing a live frozen decision re-bills - // its whole prefix as cache creation — the 11.5x regression FrozenLost exists to catch. - if err := store.ValidateIdleExit(d, o); err != nil { - return err + // The floor exists to protect 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. + // + // So it must not fire when there is no store to protect. `--store=false` / `STORE=false` + // resolves to store.Nop, which persists nothing and holds no frozen decisions, and refusing + // `--store=false --idle-exit=30m` cited a consequence that cannot occur in that configuration. + // A store-less proxy is free to exit whenever it likes. + // + // `Enabled == nil` means "not configured", which is ON — the default — so only an explicit + // false skips this. + if o.Enabled == nil || *o.Enabled { + if err := store.ValidateIdleExit(d, o); err != nil { + return err + } } if d > 0 && upstreamsPath != "" { // A self-terminating GATEWAY is a different kind of wrong: --upstreams means this diff --git a/cmd/context-guru-proxy/idleexit_test.go b/cmd/context-guru-proxy/idleexit_test.go index 9ecd36f0..0dd40325 100644 --- a/cmd/context-guru-proxy/idleexit_test.go +++ b/cmd/context-guru-proxy/idleexit_test.go @@ -233,17 +233,28 @@ func TestIdleExitStartsItsClockAtLaunch(t *testing.T) { w.mustExit("an hour after launch with no traffic at all") } -// TestIdleCheckIntervalStaysUseful pins the resolution at both ends: a 24h default must not -// mean an hour of slack past the threshold, and the 1h floor must not mean a check every few -// minutes for nothing. +// 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 }{ - {24 * time.Hour, 5 * time.Minute}, // clamped high - {time.Hour, 3 * time.Minute}, // threshold/20 - {10 * time.Minute, 30 * time.Second}, // clamped low + 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", c.threshold, got, c.want) + t.Errorf("idleCheckInterval(%s) = %s, want %s — %s", c.threshold, got, c.want, c.why) } } } @@ -327,3 +338,134 @@ func TestCheckIdleExitRefusesAGatewaySelfTerminating(t *testing.T) { } } } + +// TestCheckIdleExitSkipsTheFloorWithNoStore: the floor protects the in-memory store, so with the +// store explicitly OFF there is nothing for it to protect. +// +// `--store=false` resolves to store.Nop, which persists nothing and holds no frozen decisions. +// Refusing `--store=false --idle-exit=30m` cited a consequence — "exiting drops live frozen +// decisions and re-bills their prefix" — that cannot occur in that configuration. A store-less +// proxy may exit whenever it likes. +func TestCheckIdleExitSkipsTheFloorWithNoStore(t *testing.T) { + off, on := false, true + short := 30 * time.Minute // far below the ~5h34m floor + + if err := checkIdleExit(short, "", store.Options{Enabled: &off}); err != nil { + t.Errorf("refused a short threshold with the store disabled, citing a store that does not "+ + "exist: %v", err) + } + // Explicitly ON, and nil (= not configured, which means on) must both still be protected. + if err := checkIdleExit(short, "", store.Options{Enabled: &on}); err == nil { + t.Error("accepted a threshold below the floor with the store explicitly enabled") + } + if err := checkIdleExit(short, "", store.Options{}); err == nil { + t.Error("accepted a threshold below the floor with the store unconfigured (which is on)") + } + // The gateway refusal is independent of the store: a self-terminating gateway is wrong + // whether or not it keeps state. + if err := checkIdleExit(24*time.Hour, "/etc/context-guru/upstreams.yaml", + store.Options{Enabled: &off}); err == nil { + t.Error("a gateway with the store off may still not self-terminate") + } +} + +// 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 + h := stampActivity(http.HandlerFunc(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) + }), &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) + } + } +} diff --git a/cmd/context-guru-proxy/main.go b/cmd/context-guru-proxy/main.go index d880748d..21004d20 100644 --- a/cmd/context-guru-proxy/main.go +++ b/cmd/context-guru-proxy/main.go @@ -86,9 +86,14 @@ func main() { 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. - // Set only by the plugin installer, which pairs it with a SessionStart hook that - // starts the proxy again on demand — self-kill without that resurrection is a - // footgun, so they ship together. The floor is enforced below, not documented. + // + // 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)") @@ -236,6 +241,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, 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 — @@ -596,17 +614,6 @@ func main() { slog.Warn("context-guru: OBSERVE MODE — requests are forwarded UNMODIFIED; " + "/stats reports what compaction WOULD have saved under potential_*/projected_* keys") } - // Idle-exit validation goes BEFORE the "listening" line, because a fatal here used to be - // logged after it: the operator saw `context-guru-proxy listening` and then an exit, which - // reads as a crash rather than as a rejected configuration. - // - // Refused rather than warned about: a threshold below the store's entry lifetime does not - // degrade gracefully, it re-bills live prefixes as cache creation (the 11.5x regression - // FrozenLost exists to catch). A misconfigured value must not start. - if err := checkIdleExit(*idleExit, *upstreamsPath, cfg.Store); err != nil { - log.Fatalf("context-guru: %v", err) - } - // The sink last, so it is the line just above the traffic: "where are the logs and // what level am I getting" is the first question when something looks quiet. ln, err := listenAndAnnounce(addr, "pipeline", cfg.Pipeline, "mode", mode, "logs", sink) @@ -1021,11 +1028,34 @@ func envInt(key string, def int) int { } // envDuration reads a Go duration environment variable (e.g. "72h"). +// envDuration reads a duration from the environment, falling back to def. +// +// A NON-EMPTY value that does not parse is fatal, and that is a deliberate change from silently +// falling back. The failure it prevents is specific: `IDLE_EXIT=86400` or `IDLE_EXIT=24` — the +// natural mistake for something documented as a duration — parsed as an error, returned the +// zero default, and meant "never exit". Nothing said so, because the `idle-exit armed` line is +// only logged for a value above zero, so the operator's evidence was the ABSENCE of a line. +// +// Fatal rather than a warning: every caller here is a timeout, a retention window or a process +// lifetime. A typo in any of them silently changes behaviour in a direction nobody chose, and a +// warning in a startup log is not something anyone reads on a laptop. 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 { + log.Fatalf("context-guru: %s=%q is not a duration (%v). Use a unit — 24h, 30m, 1500ms — "+ + "or unset it to accept the default of %s.", key, os.Getenv(key), err, def) } - return def + return d +} + +// 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/proxy/ccbody_test.go b/proxy/ccbody_test.go index dac15980..a341af4c 100644 --- a/proxy/ccbody_test.go +++ b/proxy/ccbody_test.go @@ -6,7 +6,10 @@ package proxy_test import ( "encoding/json" + "io" + "net/http" "strings" + "sync" "testing" ) @@ -58,3 +61,38 @@ func claudeCodeBodyWithFirst(t *testing.T, stream bool, first string) []byte { } return []byte(body) } + +// recordedRequest carries what an upstream handler saw back to the test goroutine, with the +// synchronisation the race detector needs. +// +// The pattern it replaces looked ordered and was not: a `var forwarded []byte` written inside the +// handler and read after the round trip. The response arriving does not establish a happens-before +// edge across the loopback connection, so `-race` — which `make cover` runs — can flag it. It +// passed on every run so far, which is the least reassuring property a racy test has. +// +// A mutex rather than a channel: several tests read the capture more than once, and a mutex reads +// the same way at each use site. +type recordedRequest struct { + mu sync.Mutex + body []byte + path string +} + +func (r *recordedRequest) record(req *http.Request) { + b, _ := io.ReadAll(req.Body) + r.mu.Lock() + defer r.mu.Unlock() + r.body, r.path = b, req.URL.Path +} + +func (r *recordedRequest) forwarded() []byte { + r.mu.Lock() + defer r.mu.Unlock() + return r.body +} + +func (r *recordedRequest) requestPath() string { + r.mu.Lock() + defer r.mu.Unlock() + return r.path +} diff --git a/proxy/conformance_test.go b/proxy/conformance_test.go index c2369bba..b0159637 100644 --- a/proxy/conformance_test.go +++ b/proxy/conformance_test.go @@ -159,9 +159,9 @@ func TestCachePresetDoesNotBufferSSE(t *testing.T) { // 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 forwarded []byte + var rec recordedRequest upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - forwarded, _ = io.ReadAll(r.Body) + rec.record(r) w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"type":"message","usage":{"input_tokens":1}}`) })) @@ -193,6 +193,7 @@ func TestCachePresetNeverAddsACacheControlBreakpoint(t *testing.T) { t.Fatal(err) } resp.Body.Close() + forwarded := rec.forwarded() if len(forwarded) == 0 { t.Fatal("upstream received nothing") } @@ -260,9 +261,9 @@ func TestCachePresetLeavesTheAttributionBlockUntouched(t *testing.T) { attributionText + "\nCurrent branch: whatever the user was talking about\n"}, } { t.Run(c.name, func(t *testing.T) { - var forwarded []byte + var rec recordedRequest upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - forwarded, _ = io.ReadAll(r.Body) + rec.record(r) w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"type":"message","usage":{"input_tokens":1}}`) })) @@ -281,6 +282,7 @@ func TestCachePresetLeavesTheAttributionBlockUntouched(t *testing.T) { t.Fatal(err) } resp.Body.Close() + forwarded := rec.forwarded() if len(forwarded) == 0 { t.Fatal("upstream received nothing") } diff --git a/proxy/counttokens_test.go b/proxy/counttokens_test.go index 14e73518..880f32fa 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 rec recordedRequest upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - gotPath = r.URL.Path - gotBody, _ = io.ReadAll(r.Body) + rec.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 := rec.requestPath(), rec.forwarded() if gotPath != "/v1/messages/count_tokens" { t.Errorf("upstream path = %q, want /v1/messages/count_tokens", gotPath) } From 0177df86fb7c048b29f6110c9eb1485b27d82796 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Thu, 3 Sep 2026 16:08:00 +0300 Subject: [PATCH 4/6] test: use #191's upstreamCapture instead of a second capture helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #191 landed a mutex-guarded `upstreamCapture` in package proxy_test while this branch carried `recordedRequest`, added for the same reason on the same review. Two near-identical helpers in one package is duplication a reader has to reconcile, so this drops mine. Theirs is the better of the two: it records every round with method, path, headers and body — so a test needing the second round's headers has somewhere to read them — and `record` returns the 1-based round number, which removes the captured counter a per-round handler would otherwise need. Mapping: rec.forwarded() -> up.body(1), rec.requestPath() -> up.round(1).path. The reason both existed is unchanged and worth keeping in view: a fixture handler runs on the test server's goroutine, and the HTTP round trip that follows is not a happens-before edge. Note that `-race` reports none of these, before or after — #191 established that with a control, so this is synchronisation by the memory model rather than by anything the detector demanded. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- proxy/ccbody_test.go | 38 -------------------------------------- proxy/conformance_test.go | 12 ++++++------ proxy/counttokens_test.go | 6 +++--- 3 files changed, 9 insertions(+), 47 deletions(-) diff --git a/proxy/ccbody_test.go b/proxy/ccbody_test.go index a341af4c..dac15980 100644 --- a/proxy/ccbody_test.go +++ b/proxy/ccbody_test.go @@ -6,10 +6,7 @@ package proxy_test import ( "encoding/json" - "io" - "net/http" "strings" - "sync" "testing" ) @@ -61,38 +58,3 @@ func claudeCodeBodyWithFirst(t *testing.T, stream bool, first string) []byte { } return []byte(body) } - -// recordedRequest carries what an upstream handler saw back to the test goroutine, with the -// synchronisation the race detector needs. -// -// The pattern it replaces looked ordered and was not: a `var forwarded []byte` written inside the -// handler and read after the round trip. The response arriving does not establish a happens-before -// edge across the loopback connection, so `-race` — which `make cover` runs — can flag it. It -// passed on every run so far, which is the least reassuring property a racy test has. -// -// A mutex rather than a channel: several tests read the capture more than once, and a mutex reads -// the same way at each use site. -type recordedRequest struct { - mu sync.Mutex - body []byte - path string -} - -func (r *recordedRequest) record(req *http.Request) { - b, _ := io.ReadAll(req.Body) - r.mu.Lock() - defer r.mu.Unlock() - r.body, r.path = b, req.URL.Path -} - -func (r *recordedRequest) forwarded() []byte { - r.mu.Lock() - defer r.mu.Unlock() - return r.body -} - -func (r *recordedRequest) requestPath() string { - r.mu.Lock() - defer r.mu.Unlock() - return r.path -} diff --git a/proxy/conformance_test.go b/proxy/conformance_test.go index b0159637..084ad9a9 100644 --- a/proxy/conformance_test.go +++ b/proxy/conformance_test.go @@ -159,9 +159,9 @@ func TestCachePresetDoesNotBufferSSE(t *testing.T) { // 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 rec recordedRequest + var up upstreamCapture upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - rec.record(r) + up.record(r) w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"type":"message","usage":{"input_tokens":1}}`) })) @@ -193,7 +193,7 @@ func TestCachePresetNeverAddsACacheControlBreakpoint(t *testing.T) { t.Fatal(err) } resp.Body.Close() - forwarded := rec.forwarded() + forwarded := up.body(1) if len(forwarded) == 0 { t.Fatal("upstream received nothing") } @@ -261,9 +261,9 @@ func TestCachePresetLeavesTheAttributionBlockUntouched(t *testing.T) { attributionText + "\nCurrent branch: whatever the user was talking about\n"}, } { t.Run(c.name, func(t *testing.T) { - var rec recordedRequest + var up upstreamCapture upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - rec.record(r) + up.record(r) w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"type":"message","usage":{"input_tokens":1}}`) })) @@ -282,7 +282,7 @@ func TestCachePresetLeavesTheAttributionBlockUntouched(t *testing.T) { t.Fatal(err) } resp.Body.Close() - forwarded := rec.forwarded() + forwarded := up.body(1) if len(forwarded) == 0 { t.Fatal("upstream received nothing") } diff --git a/proxy/counttokens_test.go b/proxy/counttokens_test.go index 880f32fa..9e9cd98c 100644 --- a/proxy/counttokens_test.go +++ b/proxy/counttokens_test.go @@ -18,9 +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 rec recordedRequest + var up upstreamCapture upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - rec.record(r) + up.record(r) w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, `{"input_tokens":4321}`) })) @@ -43,7 +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 := rec.requestPath(), rec.forwarded() + 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) } From 93677f6fa7426792ef546db0c5ce0d47a192eb42 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Thu, 3 Sep 2026 18:32:06 +0300 Subject: [PATCH 5/6] =?UTF-8?q?fix(dist):=20act=20on=20the=20second=20#141?= =?UTF-8?q?=20review=20=E2=80=94=20three=20of=20these=20were=20my=20own=20?= =?UTF-8?q?fixes'=20blast=20radius?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings plus two notes. The reviewer's framing is the useful one: three are consequences of the FIRST review's fixes, so they read as follow-ups rather than fresh ground. All three were mine, and in each case the fix was right and its blast radius was not measured. ## The `envDuration` fatal broke `--version` and `--help` Every call site is a default expression in main's `var (...)` block, which Go evaluates BEFORE `flag.Parse` — so exiting from inside that helper ran before the `--version` short-circuit existed to be reached. `IDLE_EXIT=86400 context-guru-proxy --version` exited 1 with a parse error: precisely the unitless mistake the fatal was added to catch, defeating the reason `--version` was added in the same round ("an installer must be able to ask what it is"), and failing the release workflow's own `--version | grep` gate in any shell exporting a bad value. It also died before `logging.Setup()`, so the message never reached CG_LOG_FILE. The blast radius was wider than IDLE_EXIT: the helper also backs UPSTREAM_HEADER_TIMEOUT, DASHBOARD_RETENTION and the three ARCHIVE_* windows, so a deployment carrying a malformed value for any of them refused to start where it had previously fallen back. Keep the loudness, move the moment: `envDuration` records the bad value and returns the default; `checkEnvDurations` refuses after `flag.Parse`, past the `--version` return, once the log sink exists and before anything is opened. All of them at once — an operator fixing one typo should not restart to discover the next. ## `/healthz/` was not exempt, so a trailing-slash probe kept the process alive forever An exact `r.URL.Path` match. `http.ServeMux` answers `/healthz/` with a 301 to `/healthz`, which a Kubernetes httpGet probe and most monitoring loops read as healthy — and because the stamp is taken before the mux sees the request, such a probe refreshed the activity clock indefinitely. Silently, with `idle-exit armed` as the only log line: the exact failure the previous round's fix was written for, reachable by adding one character to a probe URL. `stampActivity` now asks the mux which pattern matches rather than comparing the path. `ServeMux.Handler` reports, for an internally-generated redirect, the pattern that will match after following it — so `/healthz/` resolves to `/healthz` — and reports the EMPTY pattern for an unmatched path, so a 404 (a port scanner, a stray `/health`, a typo) is no longer mistaken for use. One question to the same matcher that will route the request, instead of a second copy of the rules. ## Skipping the floor with the store off removed it entirely, and could panic `ValidateIdleExit` was not called at all with `--store=false`, so no floor applied — not even the bare 1h term. `STORE=false --idle-exit=10ns` then reached `time.NewTicker` with `threshold/20 == 0`, which PANICS: an intended startup refusal became a crash. It also broke the invariant README and docs/reference/config.md state unconditionally. Only the `2 x store.ttl_seconds` term is about the store. The 1h minimum now always applies, and `idleCheckInterval` cannot return a non-positive value regardless of caller — a crash is the wrong failure mode for a helper, and the previous version of that function reasoned "checkIdleExit refuses anything under an hour" and was then reached with 10ns through the path I had just opened. ## And - `release.yaml` was missing `-p 1`, which `ci.yaml` one file over documents as the mitigation for a real flake (#163) on a 2-core runner. This workflow runs the full suite too, so the contention is at least as bad, and a tag push must not fail to publish for a diagnosed cause. - A stale duplicate doc line above `envDuration`, left by the previous edit. - `pendingPings`'s doc said "a live entry is by construction one we intend to ping". Nearly true: `sweep` also drops entries whose `pingable()` has gone false and only looks once `Idle` has elapsed, so inside that window this counts an entry that will never be pinged. Narrowed to "outside that window", with why erring toward counting is still right — over-counting delays an exit by minutes, under-counting kills the process in the gap the keep-alive exists to work in. - `docs/how-to/use-with-claude-code.md` now says what the no-API-key path actually hands over: the proxy receives the claude.ai OAuth credential on every request, retains it in memory for a tracked session when keep-alive is on, and those pings spend the same usage limits. The section below it is titled "Keep the API key out of Claude Code", so a reader could otherwise conclude the subscription path gives the proxy less. ## A test of mine argued for the defect The first attempt at this round failed on `TestCheckIdleExitSkipsTheFloorWithNoStore` — my own test from the previous round, whose NAME asserts the behaviour this review showed was wrong. It was deleted rather than edited into shape, and its still-valid cases folded into the test named for the corrected behaviour. A test named for a defect is worse than no test: it argues for the defect on every future read. ## Verification Three new tests, each revert-verified against the pre-fix file: exact-path probe match -> TestProbeExemptionSurvivesATrailingSlash FAIL on all five: /healthz/, /metrics/, //healthz, /health (404), /nope (404) all counted as activity floor skipped entirely -> TestCheckIdleExitKeepsTheOneHourMinimumWithNoStore FAIL accepted --idle-exit=10ns, 1ms and 30m with the store disabled no panic guard -> TestIdleCheckIntervalIsAlwaysPositive FAIL idleCheckInterval(0s) = 0s; time.NewTicker would panic Finding 1 is verified by reading rather than by test: asserting it needs a subprocess that runs the built binary with a bad env var and `--version`, which felt disproportionate — the fix is that the refusal now sits after `flag.Parse` and after the `--version` return, which is a position, not a behaviour a unit test can observe. `go build ./...`, `go vet ./...`, `gofmt -l` and the full `go test ./...` are clean. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- .github/workflows/release.yaml | 6 +- cmd/context-guru-proxy/idleexit.go | 106 ++++++++++----- cmd/context-guru-proxy/idleexit_test.go | 170 ++++++++++++++++++------ cmd/context-guru-proxy/main.go | 49 +++++-- docs/how-to/use-with-claude-code.md | 8 ++ proxy/keepalive.go | 16 ++- 6 files changed, 266 insertions(+), 89 deletions(-) diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index e93fc89e..bdc4786c 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -63,7 +63,11 @@ jobs: - name: Test the shipped configuration (CGO off, no race detector) env: CGO_ENABLED: "0" - run: go test ./config/... ./components/... ./apply/... ./proxy/... ./store/... + # -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 diff --git a/cmd/context-guru-proxy/idleexit.go b/cmd/context-guru-proxy/idleexit.go index b5d96046..fa92b0a1 100644 --- a/cmd/context-guru-proxy/idleexit.go +++ b/cmd/context-guru-proxy/idleexit.go @@ -3,6 +3,7 @@ package main import ( "fmt" "net/http" + "strings" "sync/atomic" "time" @@ -56,24 +57,35 @@ func (a *activityClock) last() time.Time { return time.Time{} } -// probeRoutes are the paths that do NOT count as use. +// 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, and the session hook's -// own start-up check. Counting them was a bug that disabled the whole feature rather than -// weakening it — measured: a proxy with a 1h threshold logged -// `idle-exit armed after=1h0m0s`, 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 logs nothing to say so. +// 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 is a deliberate asymmetry — a probe is not a viewer. +// 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 +} + // 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 @@ -87,14 +99,28 @@ var probeRoutes = map[string]bool{ // 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(next http.Handler, act *activityClock, now func() time.Time) http.Handler { +func stampActivity(mux *http.ServeMux, act *activityClock, now func() time.Time) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - probe := probeRoutes[r.URL.Path] - if !probe { + // Ask the mux which pattern this request resolves to, rather than comparing r.URL.Path. + // + // An exact path compare had two holes, and the first is the one that mattered: ServeMux + // answers `/healthz/` with a 301 to `/healthz`, which a Kubernetes httpGet probe treats as + // healthy — so a probe configured with a trailing slash was counted as use and the exit + // never fired. ServeMux.Handler reports, for an internally-generated redirect, the pattern + // that will match after following it, so `/healthz/` resolves to `/healthz` here. + // + // The second: an unmatched path reports the EMPTY pattern, so a 404 — a port scanner, a + // stray `/health` probe, a typo'd URL — is no longer mistaken for somebody using the proxy. + // + // This asks the same matcher that will route the request, which is the point: no second + // copy of the routing rules to drift. + _, pattern := mux.Handler(r) + use := pattern != "" && !probeRoutes[patternPath(pattern)] + if use { act.touch(now()) } - next.ServeHTTP(w, r) - if !probe { + mux.ServeHTTP(w, r) + if use { act.touch(now()) } }) @@ -173,10 +199,20 @@ func watchIdle(o idleExitOptions) (string, bool) { // 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 { - if d := threshold / 20; d < 5*time.Minute { - return d + 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 } - return 5 * time.Minute + if d > 5*time.Minute { + return 5 * time.Minute + } + return d } // checkIdleExit is every reason a requested idle-exit threshold must not start. @@ -185,21 +221,29 @@ func idleCheckInterval(threshold time.Duration) time.Duration { // 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 string, o store.Options) error { - // The floor exists to protect 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. + // 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. // - // So it must not fire when there is no store to protect. `--store=false` / `STORE=false` - // resolves to store.Nop, which persists nothing and holds no frozen decisions, and refusing - // `--store=false --idle-exit=30m` cited a consequence that cannot occur in that configuration. - // A store-less proxy is free to exit whenever it likes. + // 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. // - // `Enabled == nil` means "not configured", which is ON — the default — so only an explicit - // false skips this. - if o.Enabled == nil || *o.Enabled { - if err := store.ValidateIdleExit(d, o); err != nil { - return err + // 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 != "" { // A self-terminating GATEWAY is a different kind of wrong: --upstreams means this diff --git a/cmd/context-guru-proxy/idleexit_test.go b/cmd/context-guru-proxy/idleexit_test.go index 0dd40325..707f96f8 100644 --- a/cmd/context-guru-proxy/idleexit_test.go +++ b/cmd/context-guru-proxy/idleexit_test.go @@ -178,12 +178,14 @@ func TestRequestsDeferIdleExit(t *testing.T) { clk := newFakeClock(time.Unix(1_700_000_000, 0)) act := &activityClock{} stampedBeforeHandler := false - h := stampActivity(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // The stamp must land BEFORE the handler runs, so a long streaming response cannot - // age out while it is still being served. + 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) - }), act, clk.now) + }) + 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{})}) @@ -270,9 +272,7 @@ func TestIdleCheckIntervalStaysUseful(t *testing.T) { func TestProbesDoNotDeferIdleExit(t *testing.T) { clk := newFakeClock(time.Unix(1_700_000_000, 0)) act := &activityClock{} - h := stampActivity(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }), act, clk.now) + 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{})}) @@ -292,7 +292,7 @@ func TestProbesDoNotDeferIdleExit(t *testing.T) { // 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(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), act2, clk2.now) + 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) @@ -339,36 +339,6 @@ func TestCheckIdleExitRefusesAGatewaySelfTerminating(t *testing.T) { } } -// TestCheckIdleExitSkipsTheFloorWithNoStore: the floor protects the in-memory store, so with the -// store explicitly OFF there is nothing for it to protect. -// -// `--store=false` resolves to store.Nop, which persists nothing and holds no frozen decisions. -// Refusing `--store=false --idle-exit=30m` cited a consequence — "exiting drops live frozen -// decisions and re-bills their prefix" — that cannot occur in that configuration. A store-less -// proxy may exit whenever it likes. -func TestCheckIdleExitSkipsTheFloorWithNoStore(t *testing.T) { - off, on := false, true - short := 30 * time.Minute // far below the ~5h34m floor - - if err := checkIdleExit(short, "", store.Options{Enabled: &off}); err != nil { - t.Errorf("refused a short threshold with the store disabled, citing a store that does not "+ - "exist: %v", err) - } - // Explicitly ON, and nil (= not configured, which means on) must both still be protected. - if err := checkIdleExit(short, "", store.Options{Enabled: &on}); err == nil { - t.Error("accepted a threshold below the floor with the store explicitly enabled") - } - if err := checkIdleExit(short, "", store.Options{}); err == nil { - t.Error("accepted a threshold below the floor with the store unconfigured (which is on)") - } - // The gateway refusal is independent of the store: a self-terminating gateway is wrong - // whether or not it keeps state. - if err := checkIdleExit(24*time.Hour, "/etc/context-guru/upstreams.yaml", - store.Options{Enabled: &off}); err == nil { - t.Error("a gateway with the store off may still not self-terminate") - } -} - // 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. @@ -408,11 +378,14 @@ func TestActivityClockKeepsItsMonotonicReading(t *testing.T) { func TestStampActivityRefreshesOnCompletion(t *testing.T) { clk := newFakeClock(time.Unix(1_700_000_000, 0)) var act activityClock - h := stampActivity(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + 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) - }), &act, clk.now) + }) + 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)) @@ -469,3 +442,120 @@ func TestParseEnvDurationRefusesAUnitlessValue(t *testing.T) { } } } + +// 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. +// +// http.ServeMux answers `/healthz/` with a 301 to `/healthz`, and a Kubernetes httpGet probe (and +// most monitoring loops) treats a 301 as healthy. With the stamp taken before the mux ever saw the +// request, such a probe refreshed the activity clock forever: --idle-exit never fired, and the only +// log line was `idle-exit armed` at startup. +// +// A 404 is the same class: a port scanner, a typo'd URL or a stray `/health` probe is not somebody +// using the proxy. Both are answered by asking the mux which pattern matches, rather than comparing +// the raw path. +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, "301 to /healthz — a k8s probe reads this as healthy"}, + {"GET", "/metrics", false, "a Prometheus scrape"}, + {"GET", "/metrics/", false, "301 to /metrics"}, + {"GET", "//healthz", false, "doubled slash, cleaned by the mux to /healthz"}, + {"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) + } + } +} diff --git a/cmd/context-guru-proxy/main.go b/cmd/context-guru-proxy/main.go index 21004d20..9b7686f7 100644 --- a/cmd/context-guru-proxy/main.go +++ b/cmd/context-guru-proxy/main.go @@ -225,6 +225,9 @@ func main() { // 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 { @@ -623,13 +626,14 @@ func main() { // Activity stamping is wired ONLY when the watchdog is on, so an ordinary deployment's // handler chain is byte-identical to before. - var handler http.Handler = h.Mux() + 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(handler, act, time.Now) + handler = stampActivity(mux, act, time.Now) slog.Info("context-guru: idle-exit armed", "after", *idleExit, "check_every", idleCheckInterval(*idleExit)) } @@ -1027,27 +1031,46 @@ 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 fatal, and that is a deliberate change from silently -// falling back. The failure it prevents is specific: `IDLE_EXIT=86400` or `IDLE_EXIT=24` — the -// natural mistake for something documented as a duration — parsed as an error, returned the -// zero default, and meant "never exit". Nothing said so, because the `idle-exit armed` line is -// only logged for a value above zero, so the operator's evidence was the ABSENCE of a line. +// 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. // -// Fatal rather than a warning: every caller here is a timeout, a retention window or a process -// lifetime. A typo in any of them silently changes behaviour in a direction nobody chose, and a -// warning in a startup log is not something anyone reads on a laptop. +// 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 { d, err := parseEnvDuration(os.Getenv(key), def) if err != nil { - log.Fatalf("context-guru: %s=%q is not a duration (%v). Use a unit — 24h, 30m, 1500ms — "+ - "or unset it to accept the default of %s.", key, os.Getenv(key), err, def) + badDurations = append(badDurations, + fmt.Sprintf("%s=%q (%v)", key, strings.TrimSpace(os.Getenv(key)), err)) + 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) { diff --git a/docs/how-to/use-with-claude-code.md b/docs/how-to/use-with-claude-code.md index 68862888..d749d95a 100644 --- a/docs/how-to/use-with-claude-code.md +++ b/docs/how-to/use-with-claude-code.md @@ -18,6 +18,14 @@ Two honest caveats: 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 diff --git a/proxy/keepalive.go b/proxy/keepalive.go index bf6d39ca..ad4f86cd 100644 --- a/proxy/keepalive.go +++ b/proxy/keepalive.go @@ -1167,10 +1167,18 @@ type KeepAliveStats struct { // 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. Everything else is gated at record time (see pingable), so a live entry is by -// construction one we intend to ping. +// 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 From 953f648b75faaaa5b7e098037fc2bbe809a4ffb3 Mon Sep 17 00:00:00 2001 From: DAVID AMID Date: Thu, 3 Sep 2026 20:09:16 +0300 Subject: [PATCH 6/6] =?UTF-8?q?fix(dist):=20act=20on=20the=20third=20#141?= =?UTF-8?q?=20review=20=E2=80=94=20a=20live=20hole,=20and=20two=20claims?= =?UTF-8?q?=20of=20mine=20that=20were=20false?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings. One is a hole I left open, two are corrections to statements I wrote, and one would have destroyed a user's files. ## Bob mode defeated the probe exemption entirely `proxy.Mux` registers a `/` catch-all whenever `BobUpstream` is set, and `--bob-upstream` did NOT trigger the gateway refusal — only `--upstreams` did. So `--bob-upstream=… --idle-exit=24h` was an accepted configuration, and in it `mux.Handler` answers pattern `"/"` for every unmatched path: `/healthz/`, `/nope`, every port-scan path. `"/"` is not a probe route, so the clock was stamped and `--idle-exit` never fired — the same silent failure the previous round's `mux.Handler` fix was written to close, reachable through a different flag. Closed by construction: the gateway refusal now covers `--bob-upstream`, so a proxy with a catch-all cannot also have a watchdog, and the message names whichever flag the operator actually passed. Plus a catch-all exclusion in the stamp decision as belt to that braces — the two rules live in different files, and if they drift, over-counting `/` as "not use" errs toward exiting a laptop proxy rather than toward a gateway that never exits. ## The mechanism I documented is not what ServeMux does I wrote that ServeMux answers `/healthz/` with a 301 to `/healthz`, and that `Handler` reports "the pattern that will match after following the redirect". Both false: `cleanPath` re-appends a trailing slash and `matchOrRedirect` only ever ADDS one, so with this route table `/healthz/` is a plain 404 and `Handler` reports the EMPTY pattern — which is what exempts it, through the same branch that exempts `/nope`. The redirect Go does generate, for a subtree root, also reports an empty pattern. The behaviour was right; the justification was invented, and it was the justification for choosing `mux.Handler` over a path compare — so anything built on it later would have been wrong. Corrected in the comment and in three test case labels that were passing for a reason they did not document. ## The refusal's advice was backwards `ValidateIdleExit` told the operator to "raise store.ttl_seconds if the short lifetime is deliberate". The floor is `max(2*ttl, 1h)`, so raising the TTL RAISES the floor: follow the advice, get the same refusal with a larger number. It also credited the floor to `2x the store's %s entry lifetime` unconditionally — with `ttl_seconds: 30` it announced `floor of 1h0m0s (2x the store's 30s entry lifetime)`, and 2x30s is 1m. The single number the operator must act on was attributed to arithmetic that does not produce it. The message now says which of the two terms binds, offers only levers that lower the floor, and when the absolute term binds it says what the 2x-TTL floor would have been so the arithmetic is checkable. This path is startup-fatal, so the message is the only evidence anyone gathers. ## The documented untar would overwrite the user's own files `archives[0].files` puts LICENSE, README.md and THIRD-PARTY-NOTICES at the archive ROOT (GoReleaser defaults `wrap_in_directory` to false), and both the release footer and the quickstart tell the evaluator to run `tar xzf …` with no `-C`. In a project directory — where somebody evaluating a proxy for their coding agent is standing — that silently overwrites their README.md and LICENSE. `wrap_in_directory: true`, and both documented commands updated to `install -m 755 context-guru_*/context-guru-proxy`. Note for the plugin PR: its install.sh locates the binary at the archive root, so it needs the same change; handled there rather than left to fail at a stranger's install. ## And The README flag table never got the `--listen` row that landed in `docs/reference/config.md`, in the same table where `--idle-exit` and `--version` were added — so a README reader could not discover the flag whose stated reason for existing is discoverability. ## Verification Two new tests, plus label corrections: TestCheckIdleExitRefusesBobModeToo — the four flag combinations, and that the message names the flag actually passed TestCatchAllRouteIsNotActivity — with a `/` route registered, /healthz/ and /nope must still not count, while the explicit Bob and Anthropic routes must TestValidateIdleExitMessageNamesTheBindingTerm — asserts the message says "2x the store's" only when that term binds, offers LOWER rather than raise, and shows the 1m0s figure when the absolute term is doing the work `go test ./...` and `gofmt -l` clean. Findings 4 and 5 are documentation and packaging, verified by reading the rendered command and the table. Assisted-By: Claude Opus 5 (1M context) Signed-off-by: DAVID AMID --- .goreleaser.yaml | 12 ++- README.md | 2 +- cmd/context-guru-proxy/idleexit.go | 58 ++++++++--- cmd/context-guru-proxy/idleexit_test.go | 131 ++++++++++++++++++++---- cmd/context-guru-proxy/main.go | 2 +- docs/get-started/quickstart-proxy.md | 5 +- store/idleexit_test.go | 43 ++++++++ store/store.go | 33 ++++-- 8 files changed, 240 insertions(+), 46 deletions(-) diff --git a/.goreleaser.yaml b/.goreleaser.yaml index 959de34a..d628b182 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -55,6 +55,14 @@ archives: 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 @@ -93,11 +101,11 @@ release: 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`: + `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-proxy ~/.local/bin/ + 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 2d1db4fc..582ae3a4 100644 --- a/README.md +++ b/README.md @@ -150,7 +150,7 @@ See [docs/components.md](docs/components.md) and [docs/reference/presets.md](doc | `--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 index fa92b0a1..4ab3850d 100644 --- a/cmd/context-guru-proxy/idleexit.go +++ b/cmd/context-guru-proxy/idleexit.go @@ -86,6 +86,21 @@ func patternPath(pattern string) string { 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 @@ -103,19 +118,25 @@ func stampActivity(mux *http.ServeMux, act *activityClock, now func() time.Time) 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. // - // An exact path compare had two holes, and the first is the one that mattered: ServeMux - // answers `/healthz/` with a 301 to `/healthz`, which a Kubernetes httpGet probe treats as - // healthy — so a probe configured with a trailing slash was counted as use and the exit - // never fired. ServeMux.Handler reports, for an internally-generated redirect, the pattern - // that will match after following it, so `/healthz/` resolves to `/healthz` here. + // 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. // - // The second: an unmatched path reports the EMPTY pattern, so a 404 — a port scanner, a - // stray `/health` probe, a typo'd URL — is no longer mistaken for somebody using the proxy. + // 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. // - // This asks the same matcher that will route the request, which is the point: no second - // copy of the routing rules to drift. + // 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)] + use := pattern != "" && !probeRoutes[patternPath(pattern)] && !isCatchAll(pattern) if use { act.touch(now()) } @@ -220,7 +241,7 @@ func idleCheckInterval(threshold time.Duration) time.Duration { // 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 string, o store.Options) error { +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 @@ -245,7 +266,7 @@ func checkIdleExit(d time.Duration, upstreamsPath string, o store.Options) error } else if err := store.ValidateIdleExit(d, o); err != nil { return err } - if d > 0 && upstreamsPath != "" { + 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. @@ -254,9 +275,16 @@ func checkIdleExit(d time.Duration, upstreamsPath string, o store.Options) error // 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. - return fmt.Errorf("--idle-exit cannot be combined with --upstreams: a gateway serving " + - "other people's agents must not self-terminate. Drop --idle-exit, or run this " + - "instance without --upstreams") + 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 index 707f96f8..4530b5b2 100644 --- a/cmd/context-guru-proxy/idleexit_test.go +++ b/cmd/context-guru-proxy/idleexit_test.go @@ -327,7 +327,7 @@ func TestCheckIdleExitRefusesAGatewaySelfTerminating(t *testing.T) { {"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) + err := checkIdleExit(c.d, c.upstreams, "", ok) switch { case c.wantErr == "" && err != nil: t.Errorf("%s: refused a valid configuration: %v", c.name, err) @@ -458,14 +458,18 @@ func probeMux() *http.ServeMux { // 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. // -// http.ServeMux answers `/healthz/` with a 301 to `/healthz`, and a Kubernetes httpGet probe (and -// most monitoring loops) treats a 301 as healthy. With the stamp taken before the mux ever saw the -// request, such a probe refreshed the activity clock forever: --idle-exit never fired, and the only -// log line was `idle-exit armed` at startup. +// 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. // -// A 404 is the same class: a port scanner, a typo'd URL or a stray `/health` probe is not somebody -// using the proxy. Both are answered by asking the mux which pattern matches, rather than comparing -// the raw path. +// 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 @@ -473,10 +477,10 @@ func TestProbeExemptionSurvivesATrailingSlash(t *testing.T) { why string }{ {"GET", "/healthz", false, "the probe itself"}, - {"GET", "/healthz/", false, "301 to /healthz — a k8s probe reads this as healthy"}, + {"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, "301 to /metrics"}, - {"GET", "//healthz", false, "doubled slash, cleaned by the mux to /healthz"}, + {"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"}, @@ -511,38 +515,38 @@ func TestCheckIdleExitKeepsTheOneHourMinimumWithNoStore(t *testing.T) { 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 { + 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 { + 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 { + 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 { + 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 { + 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 { + 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 { + if err := checkIdleExit(2*time.Hour, "", "", store.Options{}); err == nil { t.Error("accepted 2h with the store on, where the floor is ~5h34m") } } @@ -559,3 +563,94 @@ func TestIdleCheckIntervalIsAlwaysPositive(t *testing.T) { } } } + +// 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 9b7686f7..6a23de4a 100644 --- a/cmd/context-guru-proxy/main.go +++ b/cmd/context-guru-proxy/main.go @@ -253,7 +253,7 @@ func main() { // `--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, cfg.Store); err != nil { + if err := checkIdleExit(*idleExit, *upstreamsPath, *bob, cfg.Store); err != nil { log.Fatalf("context-guru: %v", err) } diff --git a/docs/get-started/quickstart-proxy.md b/docs/get-started/quickstart-proxy.md index c554a9ad..597f605a 100644 --- a/docs/get-started/quickstart-proxy.md +++ b/docs/get-started/quickstart-proxy.md @@ -8,9 +8,10 @@ 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 +# 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-proxy ~/.local/bin/ +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 diff --git a/store/idleexit_test.go b/store/idleexit_test.go index f200a9e1..0a0bd447 100644 --- a/store/idleexit_test.go +++ b/store/idleexit_test.go @@ -80,3 +80,46 @@ func TestEffectiveTTLIsWhatNewMemoryUses(t *testing.T) { } } } + +// 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 8132644f..8f224fb5 100644 --- a/store/store.go +++ b/store/store.go @@ -340,18 +340,37 @@ func IdleExitFloor(o Options) time.Duration { // 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 } - if floor := IdleExitFloor(o); d < floor { - return fmt.Errorf("idle-exit %s is below the floor of %s (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, or raise store.ttl_seconds if the short lifetime is "+ - "deliberate", d, floor, o.EffectiveTTL()) + floor := IdleExitFloor(o) + if d >= floor { + return nil } - 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