Skip to content

cg SSE peek: an adjudicate leak is counted as an expand leak in sse_expand_after_stream #174

Description

@amiddavid

What

bail() in the expand/adjudicate response loop (proxy/proxy.go) records
h.agg.RecordSSEExpandAfterStream() whenever the SSE peek withheld something and handed it back.
The counter is named for expand, but since #137 the peek's withhold set also contains
context_guru_adjudicate — so an adjudicate leak is counted as an expand leak.

How it was found

Raised in review of #137 ("as a minor side effect, bail()'s RecordSSEExpandAfterStream() call
conflates an adjudicate leak with an expand leak in that metric"). Confirmed by reading the three
bail() call sites.

Severity

Low, and accuracy-only. Snapshot.SSEExpandAfterStream is listed in notExportedWhy as
"NOT EXPORTED YET — the SSE peek's price; alert candidate", so it reaches /stats and nothing
else: no Prometheus series, no dashboard panel, no alert. Nothing acts on it today. It matters when
someone promotes it to the alert candidate it is described as, because "the peek leaked" would then
page without saying which tool leaked.

Why #137 did not fix it

Deliberately deferred there, with the reasons recorded in a comment at bail():

  1. Two of the three bail() sites cannot attribute the leak at all. A round that failed SSE
    aggregation, and a spent maxExpandRounds, both bail before anything has parsed the turn —
    there is no calls/strays split to read.
  2. A leak can be both kinds at once. In the otherTools bail the round may carry expand calls
    and adjudicate strays, so a boolean either/or split would be wrong.
  3. The honest fix is therefore a metrics change, not a one-line rename, and fix(adjudicate): declare a verdict tool and stop forcing tool_choice:none #137's review asked for
    documentation on that path rather than a behaviour change.

Fix options

  • (a) Attribute at the peek. Have sseSplicer.pass return which names matched, not just
    found bool, and count per tool. Fixes all three sites including the two that cannot parse the
    turn, and handles the both-at-once case. Costs a signature change on pass and a new counter
    family with its own render + vacuity guard (promexport.go, notExportedWhy, a
    TestSeriesRender-style guard per the TestExpandUnresolvedSeriesRender precedent).
  • (b) Rename the field to what it measuressse_proxy_tool_after_stream — and keep one
    counter. Cheapest and immediately honest, but loses the ability to tell the two apart, which is
    the thing an alert would want. Also a breaking /stats JSON key change.
  • (c) Leave it, keep the comment. Zero risk. Acceptable only while the field stays unexported;
    it must be resolved before the field is promoted to a Prometheus series or an alert.

Recommendation: (a), bundled with whatever change first exports SSEExpandAfterStream, so the
new family and its guard land together rather than exporting a knowingly-conflated number.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions