decision(extract_llm_sweep): should a locally-refused ErrNoPrefix ask count as a call? #178 says no, which steps down a published counter

[amiddavid]

@OsherElhadad — a second opinion wanted on a small accounting decision I took in #178. I think it is right, but it silently changes a published counter, so it should not rest on one person's judgement.

The change

extract_llm_sweep's prefix ask can fail with components.ErrNoPrefix — there is no stashed body from a previous turn to append the question to, which happens on every session's first turn. That is refused locally, inside proxy/prefixask.go, before any HTTP request leaves the process:

func (p prefixAsker) Ask(ctx context.Context, session, ask string) (string, components.PrefixUsage, error) {
	// …no stashed prefix for this session…
	return "", components.PrefixUsage{}, components.ErrNoPrefix
}

Before #178, metrics.RecordExtractionCall ran unconditionally after the ask branch, so this was counted as a call. In #178 I made it conditional:

// ErrNoPrefix is refused LOCALLY — there is no stashed body to append to, so no request
// leaves the process. Booking it as a call would put a 0 ms, $0 sample into the mean that
// the exploration brake reads, and inflate `calls` with work that by definition did not
// happen. Every other outcome, transport failure included, went to the provider.
if !errors.Is(err, components.ErrNoPrefix) {
	askCost = recordLeg(askMs, usage)
}

The reasoning

1. A refusal that never reaches the provider is not a call. fix(metrics): /stats credits extract_llm with 101 calls while its own log shows cands=0 on every request #176 exists because a counter named calls was reporting work a component had not done — 101 calls credited to extract_llm, which had made none. Keeping a locally-refused ask in calls is the same category of claim, just smaller.
2. It corrupts a decision path, not only a display. offload.tooSlowToExplore reads the p50 of these latencies to decide whether speculative calls are still worth their wall clock. An ErrNoPrefix refusal contributes a ~0 ms sample. First turns are common, so the brake would be reading a median dragged toward zero by events that never touched the network — and it would conclude "calls are fast" on evidence of no calls at all.
3. It distorts the derived economics. avg_latency_ms is latencyMs / calls and extraction_cost_usd / calls is how anyone reads cost-per-call. A denominator inflated by free non-events makes both look better than they are.

The cost of being right, stated plainly

This silently steps down a published counter. extract.calls (and cg_extract_calls_total{outcome="made"}) will read lower than before for the same traffic, by roughly one per session on a workload with the sweep enabled. An operator holding a baseline against the old series sees a drop with no code change they can point to, and "the component stopped working" and "the counter got honest" look identical from outside. Nothing in the payload distinguishes them.

That is a real cost and it is the reason this issue exists rather than the decision just standing.

The concrete alternative

Count it under a distinct name — refusals, or calls_refused_locally — so continuity and correctness both hold:

• calls keeps only calls that reached a provider, so avg_latency_ms, cost-per-call and the exploration brake all stay clean.
• The event stays visible and countable under its own name, so a first-turn-heavy workload is still legible, and anyone reconciling against an old baseline can add the two.
• It also separates two things the current code conflates: ErrNoPrefix (nothing to read yet — every session does this once, needs no attention) from a transport failure (sweep_ask_failed — which does). Both already have gate counters (sweep_no_prefix, sweep_ask_failed), so the naming precedent exists.

My honest read is that this alternative is better than what I shipped, and I did not do it only because it widens the surface of a PR already carrying two issues and a review round.

What would change my mind

• Evidence that anyone is holding a baseline against extract.calls or cg_extract_calls_total. If a dashboard or alert threshold is tuned to it, the continuity cost is concrete rather than hypothetical and the refusals split should land in the same release as the step-down — or before it.
• A reading of calls I have missed where "the component attempted an ask" is the intended meaning rather than "a model call was made". If that is the contract, then ErrNoPrefix belongs in it and the fix is to rename or document, not to exclude.
• Disagreement about the brake. If the ~0 ms samples are judged harmless — e.g. because exploring() gates on token volume before the latency check ever matters — then reason 2 weakens a lot and this becomes purely cosmetic, which would argue for continuity over purity.

What would not change my mind: that the old behaviour was longstanding. #176 was longstanding too.

Provenance

From #178 (fix/extract-attribution), which fixes #176 and #177. The relevant code is components/offload/extract_sweep.go's adjudicate, and the decision is recorded in the comment quoted above and in that PR's body and review thread.