perf(extract_llm): economic gate, global result cache, and the honest verdict (#28) - #34
perf(extract_llm): economic gate, global result cache, and the honest verdict (#28)#34OsherElhadad wants to merge 3 commits into
Conversation
…obally extract_llm is the only component that spends money to save money, and on Terminal-Bench it lost: 271 calls, $3.26, ~1,592s of added latency against ~197,548 unique tokens saved — ~8x underwater once those tokens are priced at the rate they would actually have been billed. ~93% of its realized value came from the replay cache, not from the LLM. The cause is arithmetic. A request to a caching backend is ~99.95% cached, so a token removed from a cached region saves the cache-read rate ($0.30/MTok), not the fresh-input rate ($3/MTok) — a 10x haircut. Break-even for one ~$0.012 call is therefore ~12,700 tokens of output under caching versus ~1,270 without it, and most tool outputs are nowhere near that. The component already carried this insight as a comment on skip_file_reads; this turns it into an actual gate that applies to every candidate. Economic gate (on by default): call the LLM only when expected_saving = removed x (1 + expected replays) x per-token value exceeds the observed mean cost of a call. The per-token value comes from Ctx.CacheAware, the compression ratio is learned from accepted results (repeated misses drive it to zero and shut the gate), and the call cost is computed from real token usage times real model pricing (CHEAP_MODEL_PRICE_*), never a hard-coded constant. Every decision records a reason. Global result cache: an extraction is a context-free derived result, so re-key it on sha256(content + prompt version + model + config fingerprint) with no session prefix — 82 of 103 unique contents recurred across sessions, and the old key threw that reuse away. A version, model, or config change misses rather than serving a stale extraction. Old entries are inert once; a session-scoped entry is still honored as a migration read. (Contrast #27's xdedup index, session-scoped on purpose: that makes a conversational reference, this caches a derived result.) Derived triggering: no per-workload threshold. Context pressure plus growth rate replace min_tokens, which stays honored when set explicitly. A merely growing context no longer fires on every step. Prompt-cache the preamble, with the measurement that matters: the ~1,463-token invariant contract now goes in a stable system block with cache_control (a leading system message on OpenAI, which has no explicit breakpoints). Measured against the gateway, this is INERT on claude-haiku-4-5, whose minimum cacheable prefix is 4096 tokens — a sub-minimum breakpoint is silently ignored, write=0 read=0. It caches on claude-sonnet-5 (minimum 1024). Shipped because it is free and correct where it wins, but /stats exposes prompt_cache_read_tokens so nobody infers a cache win from placement. Reusing the agent's cached prefix (part B) prototyped and REJECTED, with numbers: it works mechanically (a 103,019-token prefix read from cache, no write, no invalidation), but costs $0.034 at 103k, $0.153 at 500k and $0.513 at 1.7M tokens — 8.5x to 128x a dedicated cheap-model call, and ~$2.04/turn at 4 concurrent calls. It also risks a cache-write on the agent's own prefix (11.5x a read) and couples the compaction model to the agent model. Metrics: /stats gains an extract block with calls, calls avoided by cache, calls suppressed by the gate, prompt-cache behavior, cost, gross value and NET value after cost — the honest headline, previously impossible to see because cost was reported in a field separate from savings. Purely additive; every existing key keeps its name so deploy/harbor/*.py keeps parsing. Tests cover the cached-system-block shape on both backends, cross-session cache reuse, a version bump missing rather than serving stale, gate suppression when cache-aware, permission on a non-caching backend and for recurring content, the cost model against known tokens times known price, the documented break-even sizes, and that the trigger does not fire every step on a growing context. Refs #28 Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
Benchmarking the gate on real captures found two deadlocks of the same shape: a pessimistic estimate suppressing every call, so nothing was ever observed, so the estimate could never correct itself. A gate that cannot revise its own prior is an off switch, not a gate. 1. Flat per-call cost. The gate priced every call at the ~$0.012 Terminal-Bench average — roughly 5x the true cost on a workload with small outputs — and suppressed everything. Observed cost was $0.0024/call, and calls that were ~2.2x profitable were declined. Cost is now analytic and size-aware (preamble 1,463 tok + shown content + overhead, at real rates), reconciled with the observed mean once real calls exist, so it is right on the first call with no observations at all. 2. Pessimistic compression ratio. The default was 0.45; measured on real captures an accepted extraction removes only 31-254 tokens per call on 400-2,000-token outputs — a real ratio near 0.12. Corrected, with the direction of conservatism made explicit: for a SPENDING gate, conservative means UNDER-estimating the saving. But a pessimistic prior on a workload below break-even then suppressed everything and forwent a genuine +$0.0094 net, so the tracker now gets a bounded budget of 3 exploratory calls to learn whether this workload actually compresses. Also fixes /compact hard-coding the context window as unknown, which silently disabled every fraction-based trigger AND the new pressure-based triggering on that endpoint — so offline replay/eval measured a different component than ships. Measured after these fixes (replay of real captures, aws/claude-haiku-4-5; forced = pre-#28 behavior, gated = new default): Terminal-Bench, non-caching: net +$0.0287 vs +$0.0091 (3.2x), 17,286 vs 6,664 tokens saved, 4,654 vs 10,287 ms avg latency SWE-bench, non-caching: 13 calls -> 2, net -$0.0571 -> -$0.0016 (-97% waste) Both, caching backend: loss reduced and latency ~30% lower, but still NEGATIVE — the gate cannot make the component pay when saved tokens bill at the cache-read rate The honest verdict, now documented: on a non-caching backend extract_llm earns its place; on a caching backend it does not, even after #28. Docs recommend codesafe (no LLM pass) or dropping extract_llm from codesmart for caching traffic. Break-even sizes are pinned by test so a drift in these figures fails CI rather than silently invalidating the documented verdict. Refs #28 Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
OsherElhadad
left a comment
OsherElhadad
left a comment
There was a problem hiding this comment.
Independent review — verdict: do not merge as-is. Fix H1–H3 first.
I re-derived the economics myself and ran the suite. The headline conclusion holds and the
engineering is mostly careful, but there are three defects that change what the numbers mean —
one of which makes the gate's own reuse signal fire on itself.
What I verified independently (not taken on trust)
- Break-even arithmetic is CORRECT. Re-derived from scratch:
cost(s) = (1463 + min(s,5000) + 200)/1e6 × $1.00 + 200/1e6 × $5.00, value= s × 0.12 × 7 × per-token. At $0.30/MTok cached I get 30,500 tokens; at $3.00/MTok fresh, 1,800. Both match the docs and the pinned test exactly.TestBreakEvenSizesMatchTheDocumentedVerdictgenuinely fails on drift (I confirmed the bands are tight: 26k–35k against an actual 30,500, and it recomputes rather than asserting a constant). go build -tags cg_skeleton ./...✅ ·go test -tags cg_skeleton ./...✅ ·go test -race -count=5oncomponents/... internal/extract/... internal/cheapmodel/... metrics/...✅ (no races;ratioTrackeris mutex-guarded andexploring()correctly increments under the lock) ·make lint(go vet) ✅.- No credentials, tokens, or private gateway URLs introduced by this diff. The
sk-proxyliterals and IBM gateway hostnames indeploy/harbor/*are pre-existing and are placeholders/base URLs, not secrets. PR body is clean. /statsis additive-only;TestSnapshotJSONKeysAreBackwardCompatiblepins every keydeploy/harbor/measure.py:140-146andanalyze.py:90actually read.extractisomitempty. Confirmed by grep of the readers.min_tokensback-compat: the raw-YAMLprobebefore defaults is the right technique, andcodesmartsetsmin_tokens: 3000explicitly (config/config.go:160), so the shipped default preset keeps its old trigger. Good.SystemModeldegrades correctly:completeSplitconcatenates for a non-implementing client,Completeomitssystementirely, OpenAI invents nocache_control. Anthropic request shape is valid against the real API contract, and one system block cannot exhaust the 4-breakpoint cap (this is a separate request from the agent's, so no interaction withcacheinject).- The A finding is plausible and I'd accept it. haiku-4-5's minimum cacheable prefix is 4096 tokens; sonnet-class is 1024. A 1,463-token preamble is therefore genuinely inert on haiku and genuinely caches on sonnet.
write=0 read=0is exactly the documented failure mode (silently ignored, no error).prompt_cache_read_tokensis properly wired end-to-end (anthropic.go→recordUsageCache→CacheUsage()→proxy.go:571→/stats), and OpenAI'scached_tokensis correctly subtracted out ofprompt_tokensso the two backends mean the same thing.
I independently agree with "net-negative on caching backends." The 15× gap between a 30,500-token break-even and a 2,053-token largest-observed output is not a tuning problem — it is structural, and no prior correction closes it.
H1 (high) — markSeenContent poisons the gate's own recurrence signal
components/offload/extract_llm.go:388 marks content seen immediately after the gate allows it, in the same pass. But hasSeenContent is the input to expectedReuses(seenBefore=true) → 6 vs 4, a 40% valuation bump — and the mark is written on first sight, before any evidence of recurrence exists.
The mark is also global and, per state.go:80, has no TTL of its own beyond the store's. So the first encounter of any content permanently reclassifies it as "recurring", and every later encounter — in any session — reads at the inflated 6-reuse rate. That inflation is only visible in the direction that spends money.
Worse, TestGatePermitsHighReuseContent is built on exactly this distinction (34,000 tokens is chosen to sit between the recurring and non-recurring break-evens), so the one test that exercises recurrence never exercises the production path that sets the flag.
Concretely: evaluateGate decides at 34,000 tokens that recurring content pays and non-recurring does not. In production every candidate that survives to the gate is marked, so on its second appearance a 34,000-token output that has recurred exactly zero times reads as seenBefore. The gate then permits a call the arithmetic says loses.
Fix: mark content seen when it is observed, not when it is selected — move markSeenContent above the gate (or above the floor check) so the flag means "encountered before" rather than "the gate liked it before". Better: store a count and require ≥2 sightings before treating it as recurring, since "seen twice" is the actual evidence expectedReuses claims to be using. As written the docs' 82/103 cross-session recurrence rate is being asserted a priori rather than measured.
H2 (high) — the exploration budget is per-process, not per-session or per-workload
maxExploreCalls = 3 (extract_econ.go:307) is enforced on ratioTracker.explored, and the tracker is a field on the ExtractLLM value built once by config.Build and held for the process lifetime (proxy.go:108 stores one *Pipeline). So:
- The budget is global across every session the proxy ever serves. A long-lived proxy spends its 3 exploration calls on the first session's traffic and every subsequent session inherits a prior it can never revise. That is precisely the self-justifying-prior failure the PR says it fixed — the fix bounds the loop but reintroduces it at process scope.
minRatioSampleTokens = 1500is reached by one medium output. A single 2,000-token candidate ends exploration permanently, so the "learned" ratio is an n=1 estimate for the entire process.TestGateExploresThenSettlesasserts the budget is bounded but never asserts it is sufficient, andTestRatioTrackerLearnsFromObservationsdeliberately uses a 20,000-token observation, so nothing covers the n=1 case.- The ratio is a single global mean. Two interleaved workloads — one compressible JSON traffic, one incompressible file reads — average to a number wrong for both, with no way to separate them.
Note the direction of the over-fire risk you asked me to check: ratio() is removed/total over accepted extractions only. A workload where the first observation compresses well (say a JSON array at 0.5) latches a ratio 4× the measured prior, which multiplies expected saving 4× and drops the cached break-even from 30,500 to ~7,000 — for every session afterwards. The gate can absolutely over-fire; nothing bounds ratio() from above, and there is no test that a high early observation doesn't blow the break-even open. Add a cap (min(observed, 0.45) would bound it at the old prior) and scope the tracker per session, or require a real sample (say 10k tokens / 5 calls) before the observed ratio displaces the default.
H3 (high) — /stats net-value figure attributes the whole pipeline's savings to extract_llm
proxy.go:574 computes grossValue from snap.SavedTokens — the pipeline-wide before − after across every component (metrics.go:265). format, dedup, failed_run, cmdfilter, mask, extract, collapse all contribute. Cost, correctly, is extract_llm's alone.
So the "honest headline" is inflated by however much the free deterministic components saved — which on codesmart is most of it. On the very benchmark this PR reports, extract_llm contributed a small fraction of total savings; net_value_usd would show the component comfortably positive while the component-scoped arithmetic in this same PR proves it is negative. That inverts the PR's own conclusion in the one field an operator would actually read.
metrics/extract.go already tracks the right number — xGrossSaved, fed by RecordExtractionSaving. Use s.GrossSavedTokens × perSavedTok, not snap.SavedTokens. Note TestExtractSnapshotExposesAllCounters passes grossValue as a literal, so no test catches the wrong source at the call site — add one asserting the value derives from the extract-scoped counter.
Two smaller problems in the same block: agentFreshPerMTok/agentCacheReadPerMTok are duplicated as fresh constants in proxy.go:583-586 with a comment explaining the two layers "may legitimately be priced differently" — but nothing can price them differently, since both are compile-time constants that must agree for the numbers to be comparable. And the CacheMode == "off" test misses "auto"-resolved-to-off, so a non-caching request under auto is valued at the cached rate.
Medium
M1 — Cfg.MinKeepRatio is in the fingerprint but Floor is doubly volatile. cfgFingerprint includes Floor (extract.go:130), and extract_llm.go:255 sets extCfg.Floor from pressureFloor(...), which varies with context pressure every request. So the global cache key changes as the window fills: 0.6%→0.3%→0.15%→0.05% of window are four distinct keys for identical content and identical semantics. The cache cannot hit across pressure bands, silently discarding much of the reuse this PR exists to capture. Floor only gates which candidates are attempted, not what a given result means — drop it from the fingerprint, or snap it to a coarse bucket.
I could not construct two configs with different extractions and the same key — the fingerprint covers every Cfg field that reaches a strategy. The bug is the opposite: over-keying.
M2 — the version bump is manual, and the marker mode is missing from the key. PromptVersion = "v2" with a comment saying BUMP THIS. Nothing enforces it — a codeRules edit with no bump serves stale extractions forever, and the comment is the only guard. A cheap mechanical fix: derive the version from sha256(codeContract + codeRules + codeDeletionRules + codeExample)[:8] so it bumps itself. Separately, e.mode (parseMarkerMode) is not in the key even though apply() uses it to build the replacement text — two marker modes over the same content produce different output under one key. It's applied outside the cached value, so not stale-serving today, but it is a latent trap.
M3 — cross-session reuse is safe, but only because of an invariant nothing tests. I traced this properly. The cached value is out[k].projected, produced by runStarlark → execStarlarkSummary executing a model-written filter over the tool output alone. The Starlark program's only input is INPUT = that content. goal and keepIDs shape the program, not the recorded result, and both are derived from the requesting session's conversation. So: the program is session-influenced, and its output is cached globally under a key that does not include goal.
That is defensible for a deletion-only projection (IsContained proves the result is a subset of the content — no foreign bytes can enter), and it is the right call. But rewrite defaults to true (extract_llm.go:136), and validateExtraction explicitly skips the containment proof in rewrite mode (extract.go:268). In the default configuration a model can therefore reword content under the influence of session A's goal, and that rewording is served verbatim to session B. It is derived from B's own bytes so it isn't a disclosure, but it is A's interpretation of B's data — and the summary (putSummaryGlobal) is free-form model prose with no containment check at all.
Not a blocker, but the tenancy argument in the docs claims "context-free derived result" and that is only true in deletion-only mode. Either exclude rewrite: true results from the global namespace, or state the narrower guarantee. Add a test that a goal difference cannot change a cached deletion-only result — the reuse safety case currently rests on it and nothing pins it.
M4 — PR #37's 17.8s/0-tokens-saved case is only partly prevented. I cross-checked. The gate would suppress most of those calls (small outputs, cache-aware ⇒ below break-even), and RecordExtractionCall now makes the latency visible, so this is real progress. But three gaps remain: the exploration budget deliberately permits up to 3 calls that the arithmetic says lose, each up to llmCallTimeout = 15s (extract_llm.go:29) — that alone can reproduce ~17.8s across 2 calls, which is exactly the reported symptom; the gate prices dollars and never wall-clock, so a task against its time budget has no lever; and llmConcurrency calls can run in parallel per request. Add a latency circuit-breaker: skip the LLM path when observed mean latency exceeds a threshold, or make exploration respect a deadline. The gate as built cannot prevent the failure #37 found.
M5 — the methodology gap is real and the verdict partly rests on it. Deterministic replay isolates economics cleanly and I'd defend it for the cost question. But it cannot measure reward, and rewrite: true (the default) is lossy and unverified by construction. So the component's headline risk — removing content the agent needed — is unmeasured, while the PR's recommendation (disable on caching backends) happens to be the risk-reducing direction. For a disable recommendation that asymmetry is acceptable and I'd let it ship. It is not acceptable as support for the "clear win" on non-caching backends: that arm recommends more LLM rewriting on reward-unmeasured evidence. Either soften that claim or land one Harbor task with reward.
M6 — the /compact window fix is right but untested. proxy.go:220-230 is a genuine and important fix; without it offline eval measured a component that never fires pressure logic. There is no test that /compact resolves a window (no Windows reference in proxy/*_test.go), so it can silently regress to 0 and re-invalidate every future replay-based measurement — the exact class of bug being fixed. Also note h.opts.Windows == nil still yields window = 0, and replay.py:106/replay2.py:153 both drive /compact, so whether the benchmark numbers in this PR reflect the fix depends on Windows being wired in the replay harness. Worth stating explicitly in the PR.
Low
- L1
metrics/extract.gocounters are process-global whilexReasonsis unbounded — reasons are a closed set today, butRecordExtractionReasonis exported and takes an arbitrary string. - L2
round4inmetrics/extract.go:152reimplements rounding via int64 truncation;math.Round(f*10000)/10000is one line and correct at the boundaries. - L3
analyticBaselinehardcodes 2000 tokens as the reconciliation denominator; the0.1 < ratio < 10guard is sensible but the magic constant deserves a name. - L4
prevTokens(extract_llm.go:75) grows unbounded — one entry per session for process lifetime. Same forllmSeen. Pre-existing for the latter; the new map compounds it. - L5 The docs are genuinely good and describe implemented behaviour including the negative verdict — a credit worth stating. I could not run
mkdocs build --strict(mkdocs not installed here), so that check is unverified by me. - L6 The cache-key change invalidating existing entries once is noted (
extract.gocomment + docs). Old entries miss rather than mis-serve: keys arecg:xres:-prefixed andk1-versioned, and the pre-#28 session-scoped migration read goes through the oldresultKeypath. That part is correct.
On the shipping question
Ship the verdict and the measurement infrastructure — they are the valuable part, and the willingness to publish a negative result about your own component is the right instinct. But the component should be off by default on caching backends now, not merely documented as inadvisable: codesmart is the proxy default and still contains extract_llm, so the default configuration ships a component this PR proves is net-negative, with a doc note as the only guard. A codesafe-style default for caching traffic — or a hard CacheAware ⇒ skip unless min_tokens is explicit — matches the evidence better than a recommendation in prose.
The codesmart/aggressive/agent/general presets all still list it. If the verdict is real, the presets should reflect it.
…tion scope Three high-severity review defects, plus the shipping decision. H3 (worst): /stats credited the WHOLE pipeline's savings to extract_llm. It passed snap.SavedTokens — every component's savings — valued against extract_llm's cost alone, so on a preset like codesmart the component displayed as comfortably positive while its own arithmetic proves it negative. It inverted the conclusion in the single field an operator reads. This also invalidated our own benchmark table: the previously reported "+$0.0287 net win" on non-caching traffic was produced BY the bug. With honest attribution the best result on these captures is break-even, not profit. Docs corrected. ExtractSnapshot now takes a per-saved-token RATE and applies it internally to the component's own GrossSavedTokens, so passing a pipeline-wide total is no longer expressible. Regression test fails if anyone re-wires it. H1: markSeenContent fired after the gate ALLOWED a call, so first sight reclassified itself as recurring and collected a 50% valuation bump (6 expected reuses vs 4) it had not earned — the gate over-firing, opposite in direction to the two pessimistic priors fixed earlier. It is now test-and-set on OBSERVATION, returning the prior value; a suppressed candidate still counts as seen, which is correct since recurrence is a property of the content, not of what we spent. The flag-setting path now has tests; previously only the pre-computed bool did. H2: the exploration budget was per-PROCESS. The tracker lives on a Pipeline held for proxy lifetime, so the first session spent the whole budget and every later session inherited an unrevisable prior — the self-justifying-prior failure at process scope. Now per-session (budget 3 -> 2). The learned ratio is also shrunk toward the prior and capped: minRatioSampleTokens is one medium output, so a raw n=1 mean could drop the cached break-even from ~30,500 to ~7,000 permanently. PR #37: exploration spends wall clock as well as money, and an agent on a task deadline feels the former more (#37 measured 17.8s over 2 calls saving 0 tokens). Speculative calls now stop once observed mean latency reaches 6s. SHIPPING DECISION, in code rather than prose: extract_llm is disabled by default on prompt-caching backends (allow_on_caching_backend: true overrides). Every caching workload measured is net-negative even with a correct gate — break-even ~30,500 tokens/output against a largest-observed 2,053 — so the gate could reduce the loss but never eliminate it. codesmart is the proxy default; shipping a component our own numbers say loses money, guarded only by a doc note, is not a defensible default. On caching traffic it now makes zero calls and costs nothing. Medium items taken: - Floor dropped from the cache fingerprint except in "auto" mode. It is derived from context pressure, so it rotated the key as the window filled and discarded the cross-session reuse the key exists to capture; strategyOrder reads it only on the "auto" branch, so elsewhere it cannot change the result. - PromptVersion is now DERIVED from a hash of the prompt constants. A manual constant only works while every future editor remembers to bump it, and one omission serves extractions produced under rules that no longer exist, with no symptom. semanticsVersion remains as the manual hatch for validation-gate changes. - Cross-session reuse gated on RECOVERABILITY rather than restricted to deletion-only: a cached result can be a lossy rewrite steered by another session's goal, which is acceptable only while expand can recover the original. With marker_mode summary/off (or a non-persisting store) the global cache falls back to same-session reuse, on both the read and write side. Verified deletion-only results are always global. Gates: build, test -tags cg_skeleton, -race -count=5, gofmt -l (clean), go vet. Refs #28 Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
Orchestrator verification of the fix round — and the headline retraction is the important partAll three high-severity defects are fixed and I verified the two structural ones:
Full suite green, The retraction matters more than the fixFixing H3 invalidated this PR's original headline. The reported "+$0.0287 net win" on Terminal-Bench was produced by the bug — pipeline-wide savings credited to one component. With honest attribution the best result on the available captures is break-even, not profit. That is now three of this PR's benchmark results traced to measurement artifacts: the flat $0.012/call cost, the 0.45 compression-ratio prior, and this attribution error. Each pointed the same direction — making the component look better than it is — and each was found by continuing to check rather than by stopping at a favourable number. I want to be precise about what this PR now establishes, because it is a genuinely useful negative result and it should not be softened:
Preset note
On the medium itemsAll three taken, and the reasoning on cross-session reuse is better than what I asked for: gating on recoverability rather than restricting to deletion-only correctly identifies that the risk is a lossy rewrite steered by another session's goal, and that reversibility is the actual mitigation — with same-session fallback when the marker mode or store can't guarantee it. Deriving The 6 s latency brake also addresses #37's measured failure (17.8 s / 2 calls / 0 tokens saved contributing to a wall-clock timeout), which was the cross-PR concern. Verdict from my side: ready to merge once the remaining reviewer comments are addressed. The component ships honest, disabled where it loses money, and instrumented so the next person can tell. |
…rve merges (#49) Five PRs landed in quick succession (#33, #36, #40, #42, #43), each updating its own docs. Nobody checked they were coherent together, and several pages described behaviour that no longer exists. The largest error: every one of the nine preset compositions in docs/reference/presets.md still ended in `cacheinject`, which #36 removed from all of them in favour of the new `cachesplit` marker component. `agent`, `aggressive` and `general` were also missing `extract_llm`, `general` was documented only in prose, and `balanced` was called "the default" when the proxy has defaulted to `codesmart` for some time. Each of the eleven pipelines is now verified component-for-component against the `presets` map. `cachesplit` was a registered component with no page and no nav entry; it has both now. That was the only registered/documented gap — every other components.Register call already had one. Reference pages were the other systematic gap. docs/reference/routes.md documented 5 of the ~45 fields the `Snapshot` struct serves; it now covers all of them, grouped, including `discarded_changes`/`top_discarded` (#36), the SSE quintet (#33), the cmdfilter ledgers (#42) and the observe namespace (#43). config.md gained the `store` block, five missing env vars, and lost a `cacheinject` example. Corrections carrying evidence discipline rather than just names: - cacheinject's placement section was headed "measured, not asserted" over a simulation, and its one live post-fix reading (n=1, +7.9% cost per step, +61.9% cache-write, mechanism unexplained, 0 of 106 marks landing where the suspected mechanism requires) was buried below the favourable numbers. The simulation is now labelled as one, and the negative live reading leads. - The root cause of the discarded `tool_result` breakpoint was attributed to bifrost. It is this repo's own `toolMessage()` in `normalize`. - routes.md now warns that `saved_tokens` is cumulative: the unique totals behind the two studies are 234,119 and 15,457 tokens, 21x and 8x smaller. - cmdfilter.md said 23 filters and first-line selectors; it is 24 filters over six leading lines. The four filters predicted to matter fired zero times and apt+gcc carried ~73% of live savings — recorded as a failed prediction, not quietly dropped. - The `repairLostResult` removal and extract_llm's exclusion from freeze-repair (its replacement is a sampled model output) are now in design.md. - docs described `extract` as the LLM component. `extract_llm` is; `extract` never calls a model. Historical results pages keep their original pipeline names, annotated with what changed since, rather than being rewritten to numbers the runs did not produce. Untouched: docs/results/terminal-bench-*.md and improvement-plan.md (held on #23), and extract_llm's economics (#34's subject). mkdocs build --strict passes; no orphan pages, no dangling nav entries. Assisted-By: Claude Opus 5 Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com> Co-authored-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
|
Superseded by #51. This branch's history was rewritten to rebase onto #51 carries this PR's content plus:
Closing in favor of #51. Review discussion above still applies; all of it is carried forward. |
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
2 participants
Closes #28
extract_llmis the only component that spends money to save money, and on Terminal-Bench it lost: 271 calls, $3.26, ~1,592 s of added latency against ~197,548 unique tokens saved — ~8× underwater once those tokens are priced at the rate they would actually have been billed. ~93% of its realized value came from the replay cache, not the LLM.The cause is arithmetic. A request to a caching backend is ~99.95% cached, so a token removed from a cached region saves the cache-read rate (
$0.30/MTok), not the fresh-input rate ($3/MTok) — a 10× haircut.What's implemented
A. Prompt-cache the preamble — and the measurement that matters. The ~1,463-token invariant contract now goes in a stable
systemblock withcache_control(a leading system message on the OpenAI backend, which has no explicit breakpoints). Measured against the gateway, this is INERT onclaude-haiku-4-5, whose minimum cacheable prefix is 4096 tokens — a sub-minimum breakpoint is silently ignored:claude-haiku-4-5write=0 read=0— inertclaude-haiku-4-5write=5401thenread=5401claude-sonnet-5write=2653thenread=2653Shipped because it is free and correct where it wins, but
/statsexposesprompt_cache_read_tokensso nobody infers a cache win from placement.B. Reusing the agent's cached prefix — PROTOTYPED AND REJECTED, with numbers. It works mechanically (a 103,019-token prefix read from cache, no write, no invalidation) but cache-read is cheap, not free:
At ~1.7M contexts with 4 concurrent calls/turn that is ~$2.04/turn vs ~$0.016. Plus: risks a cache-write on the agent's own prefix (11.5× a read), and couples the compaction model to the agent model.
C. Global content-hash result cache. Re-keyed on
sha256(content + prompt version + model + config fingerprint)with no session prefix — 82 of 103 unique contents recurred across sessions. A version/model/config change misses rather than serving stale. Old entries inert once (noted); a session-scoped entry is honored as a migration read. Contrast #27's deliberately session-scoped index: reference vs derived result.D. Economic gate (on by default).
expected_saving = removed × (1 + expected replays) × per-token valuevs the analytic size-aware cost of a call. Per-token value fromCtx.CacheAware; ratio learned from results; cost from real pricing (CHEAP_MODEL_PRICE_*), never a constant. Every decision records a reason.E. Derived triggering. Context pressure + growth rate replace
min_tokens, which stays honored when set explicitly. A merely growing context no longer fires every step.F. Metrics.
/statsgains anextractblock: calls, calls avoided, calls suppressed, prompt-cache behavior, cost, gross value, and NET value after cost — the honest headline, previously impossible to see because cost lived in a field separate from savings. Purely additive; every existing key keeps its name sodeploy/harbor/*.pykeeps parsing.Two deadlocks found by benchmarking
Both the same shape — a pessimistic estimate suppressing every call, so nothing was observed, so the estimate could never correct:
Also fixed:
/compacthard-coded the context window as unknown, silently disabling every fraction-based trigger and the new pressure logic on that path, so offline eval measured a different component than ships.Benchmark results
Replay of real captures,
aws/claude-haiku-4-5.forced= pre-#28,gated= new default.Terminal-Bench (20 requests):
SWE-bench (19 requests):
The verdict — stated plainly
On a non-caching backend the gate is a clear win: net +$0.0287 vs +$0.0091 (3.2×) with 2.6× more tokens saved at half the latency; on SWE-bench it cut 13 calls to 2 and reduced waste 97%.
On a caching backend
extract_llmis still net-negative even after #28. The gate reduces the loss and cuts latency ~30%, but it cannot make the component pay when saved tokens bill at the cache-read rate and break-even is ~30,500 tokens/output against a largest-observed output of 2,053. Recommendation: disable it by default on caching backends — prefercodesafe(no LLM pass) or dropextract_llmfromcodesmartfor caching traffic. This is documented as the verdict rather than dressed up as a win.Break-even figures are pinned by test, so drift fails CI instead of silently invalidating the documented verdict.
Verification
CGO_ENABLED=1 go build -tags cg_skeleton ./...✅CGO_ENABLED=1 go test -tags cg_skeleton ./...✅go test -race✅ (the component runs concurrent per-output calls)make lint✅Docs
docs/components/extract_llm.mdrewritten (economics, triggering, caching, measured tables, honest verdict),docs/reference/config.md(pricing env),docs/reference/presets.md(codesmart/aggressivebehavior change + verdict).Assisted-By: Claude Opus 5