Skip to content

Feature/cloudflare gateway connector - #13

Open
anassg-lago wants to merge 18 commits into
mainfrom
feature/cloudflare-gateway-connector
Open

Feature/cloudflare gateway connector#13
anassg-lago wants to merge 18 commits into
mainfrom
feature/cloudflare-gateway-connector

Conversation

@anassg-lago

Copy link
Copy Markdown
Collaborator

What

  • Cloudflare AI Gateway connector: live path (wrap() auto-detects a client pointed at gateway.ai.cloudflare.com, skips billing on cf-aig-cache-status: HIT, auto-primes Workers AI pricing) and backfill path (gateway.adapters.cloudflare_gateway extracts a Logs API entry and bills Cloudflare's own metered cost via emit(usd_cost=..., event_id=...), idempotent across re-runs).
  • Model-attribution fix: OpenAI/Anthropic/Gemini adapters now prefer the response's own resolved model over the requested alias — fixes mispriced/misattributed events for aliased model names (e.g. Mistral's -latest, Gemini resolving to a dated snapshot).
  • Mistral pricing: resolves -latest aliases against Mistral's own /v1/models (union-find over its mutually-aliasing shape) so OpenRouter price lookups hit the right dated model.
  • Lazy pricing warm-up: Cloudflare Workers AI and Mistral pricing are primed reactively on the first wrap()'d call, not eagerly at SDK init.
  • Queue reliability: split permanent (4xx) vs. transient send failures; bounded shutdown drain instead of silently dropping stranded events on exit.
  • verify_ssl config option for local dev against a self-signed Lago instance.
  • README: documented the gateway connector, trimmed redundant sections and roadmap/phase markers.

Testing

  • 443 unit tests passing, 23 skipped (require live credentials).
  • Live-verified against a real Cloudflare AI Gateway (Workers AI, Anthropic, Mistral passthrough) and a real Lago account — confirmed exact current_usage event counts before/after.

…hit bugs

- extract_openai_native/extract_anthropic_native now prefer the response's own
  model over the requested alias, matching what actually served the call.
- Wrapper cache-hit detection: skip billing when a gateway (e.g. Cloudflare)
  served the response from cache, via .with_raw_response.create(...).
- New lago_agent_sdk.gateway.adapters.cloudflare_gateway: extract_cloudflare_log()
  and resolve_subscription() for the log-extraction half of a standalone
  connector, verified live across all three of Cloudflare's ingress methods
  (REST /ai/run, Unified/compat, Native binding) and across native wraps for
  Anthropic, Gemini, and Mistral through their dedicated passthrough endpoints.
…queue reliability

Follow-on to 2bb89a0 (Cloudflare AI Gateway connector). Adds:

- Gemini adapter: prefer the response's resolved model over the requested
  alias, same fix already applied to OpenAI/Anthropic in the prior commit.
  Extracted the shared resolve_model() helper to adapters/_common.py.
- Mistral '-latest' alias resolution for OpenRouter pricing lookups, via
  union-find over Mistral's mutually-aliasing /v1/models shape.
- Selective/lazy pricing warm-up: Cloudflare Workers AI and Mistral pricing
  are primed reactively on the first wrap()'d call (via _auto_prime_pricing_for),
  not eagerly at SDK init — OpenRouter is still warmed eagerly.
- EventQueue: split permanent (4xx) vs. transient send failures, bounded
  shutdown drain instead of silently dropping stranded events on exit.
- Added verify_ssl config option for local dev against a self-signed Lago.
- README: documented the Cloudflare AI Gateway connector (live + backfill
  paths), removed stale references and roadmap/phase markers.
Workers AI cached tokens were billed twice. `provider="workers-ai"` was
missing from `_INPUT_INCLUDES_CACHE_READ`, but Workers AI is only ever
reached through Cloudflare's OpenAI-COMPATIBLE endpoint, so its
`prompt_tokens` already includes `prompt_tokens_details.cached_tokens`.
With the cached portion never subtracted, those tokens were charged at the
full input rate AND again at the cache-read rate, which Cloudflare's
catalog does publish (verified live for kimi-k2.6, kimi-k2.7-code and
glm-5.2). Measured +583% overbill against a real cached call (prompt
23233 / cached 23168) at live catalog rates.

Gateway-backfilled Gemini calls could never be priced. The adapter passed
Cloudflare's own provider vocabulary through verbatim; a real entry
reports provider="google-ai-studio", which matched no vendor in
_VENDOR_MAP, so lookup_openrouter missed every time (confirmed against the
live 400-model table: miss as google-ai-studio, hit as gemini). The same
miss kept it out of _INPUT_INCLUDES_CACHE_READ, so Gemini's cache_read — a
subset of its input — was billed on top of input rather than subtracted.

A model already carrying its vendor prefix never matched. A real REST-path
log reports model="anthropic/claude-opus-4.8" with provider="anthropic",
which built "anthropic/anthropic/claude-opus-4.8". Now stripped, but only
when the prefix agrees with the resolved vendor, so the lookup stays
vendor-gated: a model naming a different vendor is still a miss.

Also: `_parse_price` raised InvalidOperation instead of returning None for
values >= 1e16, because `.quantize()` sat outside the try. That escaped
into emit()'s catch-all and dropped the event as an unknown error rather
than taking the normal "no price" path. Returning None also matches what
the JS port returns for the same inputs.

All 10 distinct (provider, model) pairs across the real captured fixtures
now resolve to a live price; three previously missed. The shared golden
fixture gains a `precomputed_cases` section carrying verbatim costs from
real gateway log entries, plus an optional `provider` on `cases` so
per-provider token semantics are pinned cross-repo.
@anassg-lago
anassg-lago force-pushed the feature/cloudflare-gateway-connector branch from 0bb4207 to 7f3d7c0 Compare August 7, 2026 09:01
`ruff format --check` is a CI gate, and these two files have been failing it
since 9cfde77, the branch's first commit — `main` is clean. Both changes are
purely cosmetic line-length wraps at the configured 110-char limit; no
behavior changes.

Unrelated to the billing fixes in the preceding commit, but the PR cannot go
green without it.

@ancorcruz ancorcruz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review notes

Six items, in the order I'd act on them. The first two I'd fix before merge; the next three are cheap hardening I'd do in the same pass; the last is a behaviour change worth double-checking against live data.

None of these are caught by the current suite — CI is green on all 6 jobs. Two of them (the id collision, the streaming Workers AI path) need a scenario the tests don't construct.

Comment thread src/lago_agent_sdk/sdk.py Outdated
Comment thread src/lago_agent_sdk/adapters/openai_native.py Outdated
source is warmed) rather than raising, since this is a hint, not a
contract."""
with self._lock:
self._openrouter_stale = True

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prime() force-flags every source stale with no TTL check, and wrap() calls it every time — so pricing_ttl_seconds is effectively ignored.

_openrouter_stale = True unconditionally (and the CF/Mistral flags below it), while maybe_refresh() (line 908) branches purely on those flags and never consults _openrouter_fetched / self._ttl. LagoSDK._auto_prime_pricing_for runs on every single wrap() (sdk.py:160).

A server doing sdk.wrap(OpenAI(...)) per request therefore re-marks every primed source stale continuously, and the queue thread re-downloads OpenRouter's full catalog on every flush_interval tick — indefinitely. maybe_refresh() also runs synchronously at the top of EventQueue._run's tick, ahead of the drain, so each of those fetches delays event delivery.

No wrong bills, just steady unnecessary work — but it's a divergence from what this docstring describes ("zero further network calls until the TTL expires") and from the CHANGELOG's claim that prime() no longer eagerly force-fetches. Gating each flag on table is None or age >= self._ttl restores the documented behaviour.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, and I'm deferring this one rather than fixing it now — flagging explicitly so it doesn't read as forgotten.

Your diagnosis matches what I see: prime() sets _openrouter_stale = True unconditionally, maybe_refresh() branches only on those flags without consulting _openrouter_fetched / self._ttl, and _auto_prime_pricing_for runs on every wrap() — so a server wrapping per request re-downloads the catalogue on roughly every flush tick, indefinitely, and each fetch sits ahead of the drain in the queue's tick.

The reason for deferring is your own framing: "No wrong bills, just steady unnecessary work." Everything else outstanding on these two PRs is a money path — the cache-key casing turned out to be a measured 3.96x over-bill, the unit basis was under-reporting an Anthropic cached call by nearly half, and the ~-alias gap billed nothing at all for 11 models. Against those, wasted bandwidth and a delayed drain are real but not urgent, and the fix touches the refresh path that every price-mode customer depends on — I'd rather land it with its own attention than bundle it into a batch of unrelated changes.

It's grouped with the other hardening items from your non-blocking list (the per-source backoff you raised alongside it, the Workers AI pagination cap, send_individually ordering, the eager undici import) for a follow-up PR per repo once the money paths are done. pricing_ttl_seconds being effectively ignored is a documented divergence from both the docstring and the CHANGELOG, so it stays on the list rather than being written off.

If you'd rather it land before merge, say so and I'll pull it forward — it isn't a large change, and gating each flag on table is None or age >= self._ttl is exactly what you described.

Comment thread src/lago_agent_sdk/sdk.py Outdated
Comment thread src/lago_agent_sdk/gateway/adapters/cloudflare_gateway.py Outdated
Comment thread src/lago_agent_sdk/adapters/gemini_native.py
`_is_permanent_failure` treated the whole 400-499 range as unretryable, but
429 (rate limited) and 408 (request timeout) both mean "try again, later".
A throttled 100-event batch therefore took the isolate-and-drop path: 100
further requests aimed at the server that had just asked us to slow down,
each also throttled, each then logged and discarded. 100 billable events
lost, and the isolation actively deepened the throttle.

This was a regression — before the permanent/transient split existed, the
same 429 went through the 1s->60s backoff and eventually landed.

"Permanent" is now an explicit set (400, 401, 403, 404, 409, 422) rather
than a range, so an unrecognized 4xx errs toward retrying: waiting on an
event that would have been dropped costs latency, dropping one that would
have been accepted costs revenue. The isolate-one-by-one recovery is
unchanged for the validation statuses it was written for, so a single bad
transaction_id still doesn't take its batch down with it.

Retry-After is deliberately not honoured yet: LagoApiError carries only
(status, body) and the raise site discards headers, so respecting it means
changing an exported constructor.
`_infer_provider` matched only a bare "@cf/...", but Cloudflare's
OpenAI-compatible /compat endpoint requires the routing-prefixed form —
"workers-ai/@cf/meta/llama-3.3-70b-instruct-fp8-fast". That is what the
README and the demo notebook prescribe, and the only form a STREAMING call
can report, since the synthetic usage payload carries no model and
`resolve_model` falls back to the requested string verbatim.

Those calls were stamped provider="openai", looked up against OpenRouter as
"openai/workers-ai/@cf/...", missed, and silently degraded to token events —
with nothing to degrade to in an llm_cost-only billing setup. The
`workers-ai` entry in _INPUT_INCLUDES_CACHE_READ never applied either, so on
the paths that did price, cache reads were billed twice again.

Recognising the prefixed spelling alone is not enough: Cloudflare's catalog
keys models as bare "@cf/...", so lookup_cloudflare_workers_ai now strips
the routing prefix before matching, and a genuine unknown model still
misses rather than becoming a false hit.

CanonicalUsage.model deliberately keeps the spelling the customer used, so
reporting stays faithful to the request while pricing resolves.
Each stream wrapper rebuilds a synthetic usage payload from the chunks, and
all three discarded the model the response reported — so `resolve_model`
fell back to the requested alias, which is precisely the bug the
non-streaming path was fixed for. A streamed "gpt-5-chat-latest" stayed
"gpt-5-chat-latest" instead of resolving to the dated snapshot OpenRouter
lists, so price mode missed and degraded to token events while the
identical non-streaming call priced correctly.

Each provider hides the resolved name somewhere different:

  * OpenAI reports `model` on every chunk, and on `response` for the
    Responses API's terminal event.
  * Anthropic reports it ONLY on `message_start` under `message.model`, so
    the wrapper now keeps it across the whole accumulate-and-merge stream.
  * Gemini reports `model_version`, which it hot-swaps server-side for
    "-latest" aliases.

It matters most on a gateway, where the resolved name decides which price
table the call is looked up in at all.

The streaming fakes carried no model field and no wrapper test asserted
properties["model"] — both now do, one attribution test per wrapper.
`_emit_token_events` and the per-field branch of `_push_cost_event` both
built `f"{event_id}_{field_name}"` over the same field vocabulary, and both
are reachable for the SAME event_id: emit() falls back to token events when
a price lookup misses, then takes the cost path once the table is warm.

So a backfill re-run over one window sent `backfill_X_input` under
llm_input_tokens on the cold pass, then re-sent the identical id under
llm_cost on the warm pass. Lago rejected it as a duplicate transaction_id,
and because /events/batch is all-or-nothing that rejection failed every
other event in the batch too. The dollar amounts for that window were never
billed — only the raw token counts — and nothing surfaced it. That defeats
the idempotency promise event_id exists to provide.

The two multi-event paths now use disjoint namespaces (`_tok_` / `_cost_`).
The single precomputed-cost event stays unsuffixed: it pushes exactly one
event, so there is nothing to disambiguate.

No migration needed. `event_id` is absent from 0.2.0 entirely, so no
released path has ever emitted the old format — they all use a random UUID.
That is also why both namespaces could be made explicit rather than only
prefixing the cost side.

Regression test asserts a cold run's ids and a warm run's ids over the same
event_id do not intersect; verified to fail without the fix, where all four
ids collided exactly.
`_pick_mistral_canonical` sorted by `(len(n), n)`, but every dated id in one
family is the same length — so the length term always tied and the choice
fell through to the alphabetical term, which for `-2402` / `-2407` / `-2411`
IS the date, ascending. A family of
mistral-large-2402/-2407/-2411/-latest collapsed onto mistral-large-2402 and
the whole family was priced at a two-year-old rate.

Now sorts by a normalized date descending. 4-digit YYMM is widened to
YYYYMM00 first, because mixed suffix widths do not compare correctly as raw
strings ("20250929" sorts below "2411").

Two further parts of the same bug:

  * An explicit dated snapshot is no longer remapped. The mapping loop
    rewrote every non-canonical member, including the real dated ids, so
    asking for `mistral-large-2411` by name was redirected to the group's
    canonical and priced at that snapshot's rate. It matched OpenRouter
    directly before alias resolution existed, so that was a regression.

  * Ordering is by Unicode code point, and the JS port's `localeCompare` is
    gone. It is ICU/locale-dependent, so it was not reproducible across
    environments, and for a group differing only by case/separator the two
    repos picked DIFFERENT canonicals: `Mistral-Small-2603` here vs
    `mistral_small_2603` in JS. `_norm` lowercases and maps "." to "-" but
    leaves "_" alone, so the JS pick normalized onto a name OpenRouter does
    not list and the group fell back to token events there while pricing
    correctly here. Both repos now agree.

Regression tests verified to fail without the fix (4 here, 5 in JS).
The cost path spread `dimensions` into `base_properties` — before `unit`,
`value`, `base_cost` and `unit_price` — so those four SDK-computed keys
overwrote a caller dimension of the same name. `_emit_token_events` spreads
`dimensions` last and honoured it. One customer config, two different
outcomes depending on the mode, and no error on either path.

`dimensions` is now spread last in both, so one rule holds: a caller
dimension always wins over an SDK-computed property of the same name.

This is a reachable config rather than a contrived one — `unit`, `value` and
`model` are ordinary words a customer may use for their own breakdown, and a
per-seat biller naturally writes dimensions={"unit": "seat"}.

The accepted consequence is now pinned by a test rather than left implicit: a
dimension named `value` overrides the REPORTED quantity on token events. It
cannot affect the amount actually charged on a cost event, because
`precise_total_amount_cents` is a sibling of `properties`, not a member.

Regression test asserts both emitters for the same dimensions dict; verified
to fail without the fix, where the cost path reported unit='150' not 'seat'.
`__init__` defaulted `api_url` to the production URL, so the `if api_url:`
guard that exists to let explicit args win ALWAYS fired and overwrote
`config.api_url`. That meant

    LagoSDK(api_key=k, config=LagoConfig(api_url="http://localhost:3000/api/v1"))

shipped a local-dev customer's usage data to api.getlago.com, silently.

The default is now None, and each explicit arg is guarded on "was it passed?"
rather than on truthiness — so a config value survives when the arg is
omitted, an explicit arg still wins when given, and the production default is
unchanged when nothing is passed.

This was the shortest path to the bug rather than an exotic one: a custom
api_url and verify_ssl=False go together in exactly one setup — a local Lago
behind a self-signed cert, which is what verify_ssl was added for — and
verify_ssl was reachable only through a LagoConfig, so the feature pushed
callers into the clobber. `verify_ssl=` is now accepted directly.

Validated against a real local Lago instance: events emitted with
`LagoSDK(api_key, api_url, verify_ssl=False)` and no LagoConfig land with the
exact expected unit deltas, and the config-only form now resolves to the
local url instead of production.
…eries test

Two test-infrastructure fixes found by actually pointing the live suite at a
real Lago instead of the in-process mock.

Reconciliation could not run against a local Lago at all. It is the ONLY test
that proves Lago *accepts* what we emit — every other integration test talks
to a mock, so a wrong metric code or a rejected precise_total_amount_cents
would pass there and surface only in production. Both halves failed on SSL
against a self-signed dev cert: the module's own `requests.get` never passed
`verify=`, and the SDK was built without `verify_ssl`. Both now honour
LAGO_VERIFY_SSL, mirroring LagoConfig.verify_ssl.

Note it had also been skipping silently for a second reason: it gates on
LAGO_EXTERNAL_SUBSCRIPTION_ID while the local .env defines
LAGO_SUBSCRIPTION_ID, so the skip never announced a misconfiguration.

The o-series reasoning test was a coin flip. `o4-mini` spends a variable
number of reasoning tokens on the same prompt — measured 0 on some calls and
non-zero on others minutes apart — and since the SDK only emits non-zero
fields, the hardcoded assertion failed and passed on identical input,
alternating between the two repos. It now asserts the SDK's actual contract —
emit reasoning tokens WHEN the provider reports them — reading the reported
count off the response, checking the emitted value matches it, and skipping
when the model answered without reasoning. Verified stable over four
consecutive runs per repo.
`unit` on the single-event path was `str(usage.input + usage.output)`, which
dropped `reasoning` and `cache_write` entirely and counted a cache-inclusive
provider's cached tokens at full weight — while the per-token_type branch
directly below reports the de-overlapped `parts["tokens"]`. Two branches of one
method, two different bases for the same call.

On the captured 16_real_gemini_via_dedicated_endpoint.json row (9 in / 21 out
/ 852 reasoning) it published unit="30" for a call that consumed 882.

New `deoverlapped_token_total()` sums the same PRICED_FIELDS the split path
emits one event each for, so the two agree by construction. The charged amount
was never affected — that comes from precise_total_amount_cents — but `unit`
is the reported quantity a customer points a sum aggregation at.

Both _INCLUDES_ sets are applied and deliberately NOT gated on a price
existing, unlike compute_cost's subtraction: this is a token count, so whether
a rate happens to be published cannot change how many tokens were consumed.
The two still agree, because a cache-inclusive provider with no cache_read
price keeps those tokens inside `input` and emits no cache_read event.

Limited to the five PRICED_FIELDS on purpose: tool_calls is a count of calls
rather than tokens, and cache_write_5m/cache_write_1h are a breakdown OF
cache_write, so neither belongs in a token total.

One under-report the review didn't name: for an ADDITIVE provider the old
basis was much wider than the reasoning case. Anthropic with input=1000,
cache_read=900, output=100 reported 1100 against 2000 consumed. Verified
against a live Lago instance — the llm_cost charge's units moved by exactly
2000, with the dynamic charge carrying the metered $0.05.
emit() returns early whenever the effective mode isn't "price", and never
consulted usd_cost on the way out — so a caller who passed a gateway's real
metered price got token counts instead, with no log and no on_error. A
hand-rolled backfill written from the module docstring (the pattern
examples/cloudflare_gateway_demo.ipynb demonstrates) would drop every real
cost it read and still look like it succeeded.

The configured mode is still respected, deliberately: honouring a per-call
usd_cost in token mode would emit an llm_cost event that maps to none of a
token-mode customer's configured charges. What changes is that the discard now
reaches on_error, the same hook every other billing gap uses.

Reported per occurrence rather than deduped — the count of discarded costs is
exactly what a caller reconciling on on_error needs, and the documented
backfill pattern passes an explicit mode="price", so hitting this at volume
means a real misconfiguration rather than normal operation. The common case,
no usd_cost supplied, stays silent.

Validated against a live Lago instance: on_error fires naming the discarded
amount, the call still bills 13/17 token units, and no llm_cost event appears.
`cache_read` checked only `input_cached_tokens` and `cache_write` only
`input_cache_creation_tokens`, while `reasoning` on the next line already
checked two casings — and that asymmetry was the tell.

Surveying every usage_metadata key across all 14 captured fixtures: the
gateway's OWN counters are consistently snake_case, but a provider's native key
can pass through untouched. The real Gemini entry carries camelCase
`reasoningTokens` plus an `input_text_tokens` this adapter maps nowhere. So the
spelling of a cache key on a provider we have no CACHED capture for was
genuinely unknown.

The consequence is an over-bill, not a lost field. `_normalize_provider` maps
these entries to `gemini`, and `gemini` is in _INPUT_INCLUDES_CACHE_READ, so
compute_cost relies on cache_read being populated to SUBTRACT the cached portion
out of input. A silent 0 leaves the whole prompt billed at the full input rate.
Measured against the live OpenRouter table on a gemini-2.5-flash call with 9,000
of 10,000 prompt tokens cached: $0.00325 against a true $0.00082, a 3.96x
over-bill — and it grows with cache hit rate, so it is worst on the
long-cached-system-prompt workload caching exists for.

Both fields now check the gateway's snake_case name, its camelCase form, and the
provider's own native name (cachedContentTokenCount for Gemini,
cache_creation_input_tokens for Anthropic). An extra spelling costs a dict
lookup; a missed one costs 4x.

Fallthrough is on any falsy value, not just a missing key, so a provider sending
both its own name and the gateway's with one zeroed still resolves to the real
count. The JS port used `??` here, which only skips null/undefined and resolved
to the zero — fixed there too.
…sion

OpenRouter marks a MOVING alias with a leading "~" on the vendor —
"~anthropic/claude-sonnet-latest", "~openai/gpt-latest",
"~google/gemini-flash-latest". parse_openrouter split the id on "/" and took the
left half as the vendor, so these indexed under "~anthropic"/"~openai"/"~google",
none of which appear in _VENDOR_MAP.

A customer in price mode requesting a plain "-latest" alias therefore missed the
table and fell back to token events — billing nothing at all in an llm_cost-only
setup. Measured live against the 415-model catalog: 11 such ids across 6
vendors, every one carrying real token pricing, every one previously
unpriceable, all 11 now resolving. Includes claude-sonnet-latest,
claude-opus-latest, claude-haiku-latest, gpt-latest and gpt-mini-latest — names
a customer plausibly asks for by hand.

The "~" id stays indexed alongside the bare one, so nothing that already worked
changes. Verified collision-free: no un-prefixed id duplicates a "~"-prefixed
one, so the strip cannot overwrite a real listing.

Separately, _VERSION_DATE_SUFFIX now also strips a 3-digit revision. Gemini's
model_version can report "-002" where OpenRouter lists only the bare name, so
preferring the resolved id turned a hit into a miss. Latent rather than live —
every captured real Gemini response reports a bare "gemini-2.5-flash" — but it
costs one regex arm and is a config change away. Verified safe: zero of the 415
live ids have a model part ending in exactly three digits.

Found while investigating the reviewer's Gemini "-002" question on #13, which
turned out to have TWO independent causes; fixing only the suffix would have
left that model missing anyway.
The catalog fetch had no test coverage at all and two ways to under-price.

`result_info.total_count` was trusted as a terminator and defaulted to
len(models) when absent, so a missing count broke after page one — keeping 50 of
the 64 models the endpoint actually serves, silently. Measured live, that count
is worse than absent-able: it reports 291 while the endpoint serves 64 (50, then
14, then 0), so `len(models) >= total` can never fire. A short page is the only
reliable end-of-catalog signal and is now the only one used. Today's behaviour
was correct by luck — the short page ended the loop — but any change to that
count would have cost 14 models with no diagnostic.

The `while True` had no page bound, on a loop that runs on the queue's flush
tick AHEAD of the drain: an endpoint returning full pages indefinitely would
stall event delivery, not just waste bandwidth. Capped at 40 pages (~2000 models
against a real catalog of 64), and the truncation is logged, since a short
catalog otherwise reads as "these models are unpriced".

Python-only: one malformed entry unpriced EVERY Workers AI model.
`m.get("properties", [])` only defaults when the key is absent, so an explicit
JSON null returned None and `for p in None` raised TypeError out of
parse_cloudflare_workers_ai into maybe_refresh's handler, which leaves the table
at None. The JS port already isinstance-guarded and dropped only the bad entry.

Five new tests cover the loop that previously had none: the real 50-then-14
shape, a missing total_count, a wrong total_count, the page bound, and a
null-properties sibling surviving. Verified live that the full 64-model catalog
is still walked and yields the same 36 token-priced models.
…from JS

Cross-port audit findings, none of them raised in review.

A dropped event now always reaches on_error rather than only the module logger.
Two paths lost billable events silently from the hook's point of view: the
queue's buffer overflow, and emit() dropping a call when no subscription
resolved. The JS port already reported both, so the two repos disagreed on
whether a lost event is visible at all.

A negative token count could be emitted as a billable quantity.
`nonzero_numeric` filtered on truthiness, so input=-100 survived and was pushed
as value="-100". Nothing upstream should produce one — every adapter clamps at
extraction — but this is the last gate before an event is built, and JS already
filtered on > 0.

`apply_markup` was the one money helper that could raise. A non-numeric input
hit a bare Decimal("abc") and threw InvalidOperation from inside
_push_cost_event, under emit()'s catch-all, so the cost event was dropped and
reported as an unknown "emit" error rather than taking the documented no-price
path — past every caller relying on this module's None-on-bad-input convention.
Now parsed through _parse_price and floored to 0, matching JS and
compute_precomputed_cost's existing behaviour.

money_golden.json is untouched and still byte-identical across repos; both
golden suites pass.

Not fixed here: `_safe_int("1825.0")` returning 0 where JS returns 1825. Every
token value in the real Cloudflare fixtures is an int, so it is unreachable on
this branch — it belongs with the Databricks table columns, which the adapter
documents as arriving as strings.

@ancorcruz ancorcruz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of the fixes (17b3005..5c514fe)

All five fixes verified in the code, and three went further than the reports asked. Streaming model-drop was fixed in all three wrappers, not just OpenAI's; the cache-key lists gained the providers' native spellings; and the pagination fix corrected my suggestion — I proposed keying on result_info.total_count, you measured it reporting 291 against 64 served and used a short page instead. I also checked deoverlapped_token_total agrees with the split path in all four provider/price combinations, and the _tok_/_cost_ regression test does fail without the fix.

Agreed on deferring prime()/TTL, for the reason you gave — it's the only one of the set that doesn't touch money. Leaving that thread open; resolving the other five.

CI green on all 6 jobs, 513 unit tests pass locally.

Four of the eleven items below are regressions introduced by these fixes, which is the main thing worth acting on: the overflow report can deadlock a customer's thread, api_url now accepts "" and silently stops all billing, deoverlapped_token_total over-reports for workers-ai, and narrowing _PERMANENT_STATUSES re-opened head-of-line blocking for 413. Six were reproduced by running them.

Smaller notes not worth their own threads:

  • _WORKERS_AI_COMPAT_PREFIX is now defined in both pricing.py:124 and openai_native.py:52, each with a comment asking a human to keep it in sync — and they're load-bearing on each other (recognise the prefix, then strip it to price). pricing.py has no cycle with adapters/, so one definition would do.
  • sdk.py:291's no-subscription path now emits logger.error and a logger.warning via _report_error — two lines per drop.
  • canonical.py's nonzero_numeric silently drops a negative count, while the same commit adds on_error reporting for the other two drop paths. Defensible since adapters clamp upstream, but it's the odd one out now.
  • verify_ssl is now a first-class LagoSDK(...) argument and the docstring recommends verify_ssl=False for local dev — which routes the documented happy path straight through lago_client.py:25's unguarded requests.packages access (on the deferred list). Worth pulling that one forward with the rest of the hardening, since it's now the advertised setup.

# every other billing gap — never learned revenue had been lost. The
# JS port already reported this, so the two disagreed on whether a
# dropped event is visible.
self._report_error(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (new): the overflow report runs the customer's callback while holding the lock, so a re-entrant callback deadlocks their thread.

push() takes self._lock at line 123, and this _report_error — which invokes config.on_error — is called inside it. self._lock is a plain threading.Lock (line 83), not an RLock. So a callback that touches the SDK in any way reaching push(), flush(), or stop() blocks on a lock its own thread already holds, and hangs forever. The surrounding try/except can't help: a deadlock isn't an exception.

This is the only one of the four _report_error call sites inside the lock — 198, 251 and 284 are all outside.

It's a plausible trigger rather than an exotic one: overflow happens under sustained load, which is exactly when someone's on_error hook would want to emit a diagnostic or force a flush. And the failure is worse than the unreported drop it was added to fix — an unnoticed dropped event costs one event, a hung producer thread costs the whole application.

Even without re-entry there's a cost: once the buffer is full, every subsequent push runs the callback plus a logger.warning synchronously, on the customer's LLM-call thread, holding the lock that all producers and the drain thread need.

Same shape as should_wake fixes it — set a flag inside the lock, report after releasing.

Comment thread src/lago_agent_sdk/sdk.py
# passed?", never on truthiness, so a config value survives when it wasn't.
self.config.api_key = api_key or self.config.api_key
if api_url:
if api_url is not None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (new): api_url="" now overwrites the production default, and the resulting failure retries forever.

The if api_url:if api_url is not None: change is right for the bug it fixes, but it swapped one silent misroute for another. LagoSDK(api_key=k, api_url=os.environ.get("LAGO_API_URL", "")) with the env var unset previously kept https://api.getlago.com/api/v1; now it writes "".

The downstream behaviour is the bad part. send_batch POSTs to "/events/batch", requests raises MissingSchema, and that isn't a LagoApiError — so _is_permanent_failure returns False, _replay_failed re-prepends the batch, and the queue retries at the 60s ceiling indefinitely. All billing stops, nothing is ever dropped or escalated, and the only symptom is a growing buffer.

if api_url: for this one field keeps your fix intact — the bug you were fixing was a truthy default overwriting config, so falsiness is the right guard here specifically. Or if api_url is not None and api_url != "" if you'd rather keep the shape uniform across the four fields.

)


def deoverlapped_token_total(usage: Any) -> int:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (new): workers-ai is in _INPUT_INCLUDES_CACHE_READ but missing from _OUTPUT_INCLUDES_REASONING, so this over-reports by 73%.

Verified by running it — identical usage, different provider:

CanonicalUsage(input=100, output=1000, reasoning=800)
  provider="openai"      -> 1100
  provider="workers-ai"  -> 1900

workers-ai is reached only through Cloudflare's OpenAI-compatible endpoint — that's the argument _INPUT_INCLUDES_CACHE_READ's own comment (line 81) makes for including it. In that payload shape completion_tokens_details.reasoning_tokens is a subset of completion_tokens, which is precisely why openai is in _OUTPUT_INCLUDES_REASONING. And extract_openai_native fills reasoning from that key with no provider gate (lines 159, 170). So the subset gets counted twice, and the new single-event unit publishes 1900 for a call that consumed 1100. Cloudflare hosts reasoning models (deepseek-r1, qwen, glm), so it's reachable.

compute_cost would double-bill the same tokens; it doesn't today only because _CLOUDFLARE_UNIT_FIELD_MAP happens to carry no reasoning unit. That's an accident, not a guard.

Adding "workers-ai" to _OUTPUT_INCLUDES_REASONING fixes the symptom. Worth considering the structural version too: lines 336-355 restate compute_cost's de-overlap (same .lower(), same PRICED_FIELDS comprehension, same two membership tests), and the stated goal is that the two paths "agree by construction". Two copies can only agree by convention — and this finding is them agreeing with each other while both diverging from what _OUTPUT_INCLUDES_REASONING is for. A token_total on CostBreakdown, set from the counts compute_cost already builds, would make it structural and drop the usage: Any signature.

# (request timeout). Treating those as permanent dropped billable events AND fanned
# one throttled batch out into up to `max_batch_size` extra requests aimed at the
# server that had just asked us to slow down.
_PERMANENT_STATUSES = frozenset({400, 401, 403, 404, 409, 422})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (new): excluding 429/408 is right, but the explicit 6-status list makes genuinely-unretryable 4xx transient again.

Rescuing 429 and 408 from the permanent path is a real fix. The cost is that 413, 402, 415, 405 and 410 are now transient, and for those re-sending the same batch provably cannot succeed.

413 Payload Too Large is the concrete one — realistic for the default max_batch_size=100 with sizeable properties dicts, and returnable by any ingress in front of Lago. send_batch raises LagoApiError(413), _is_permanent_failure says transient, _replay_failed re-prepends the identical oversized batch at the head of the FIFO, and it backs off to 60s forever. Every event behind it is blocked until the 10,000-event buffer overflows.

The sharp part: _send_individually would have split that batch into per-event sends and actually delivered them — but it's only called from the permanent branches (lines 247, 282), so the batch that most needs splitting never gets there.

The docstring's rationale ("dropping one that would have been accepted costs revenue") is sound for an unknown 4xx. It just doesn't cover statuses where the batch can't succeed as-is but its events can individually. Adding 413/402/415 to the set routes them to _send_individually, which is the behaviour you want — split, deliver what's deliverable, drop only what genuinely 4xxs alone.

# — Gemini calls it `cachedContentTokenCount`, Anthropic
# `cache_creation_input_tokens`, and the `reasoningTokens` fixture proves
# native keys do reach us unnormalized.
cache_read=_first_int(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The native-spelling coverage is asymmetric between the three fields.

The premise behind this fix — the gateway can forward a provider's own keys untouched, proven by the reasoningTokens fixture — is now applied unevenly:

  • cache_read gets Gemini's native cachedContentTokenCount, but not Anthropic's native cache_read_input_tokens.
  • cache_write (line 140) does get Anthropic's native cache_creation_input_tokens.
  • reasoning (line 145) gets neither provider's native name — no Gemini thoughtsTokenCount.

So an Anthropic entry forwarding native keys resolves cache_write correctly and cache_read as 0. Anthropic's cache is additive, so those cached prompt tokens drop out of the bill entirely — and nonzero_numeric then emits no llm_cached_input_tokens event at all. That's a straight under-bill, and it's the mirror image of the over-bill this commit fixed.

Adding cache_read_input_tokens to the first list and thoughtsTokenCount to reasoning closes it. The module docstring's "Field mapping" block (lines 8-12) still lists one spelling per field and is now stale.

# OpenRouter lists only the bare name). Verified safe against the live 415-model
# catalog: ZERO ids have a model part ending in exactly three digits, so the
# "-\d{3}" arm cannot shorten a real listing.
_VERSION_DATE_SUFFIX = re.compile(r"-(?:\d{8}|\d{3}|v\d+)$")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -\d{3} arm governs Bedrock and Cloudflare lookups too, and the safety check only covered OpenRouter.

"Zero of the 415 live ids have a model part ending in exactly three digits" is the right check — for OpenRouter. But _strip_version is also called from bedrock_model_key (line 695), _aws_model_keys (701, 705) and lookup_cloudflare_workers_ai (661), and those catalogs weren't part of it. Verified:

_strip_version("text-bison-001")             -> "text-bison"
_strip_version("amazon.titan-text-lite-100") -> "amazon.titan-text-lite"
_strip_version("@cf/baai/bge-base-en-001")   -> "@cf/baai/bge-base-en"
bedrock_model_key("amazon.titan-text-lite-100") -> "titantextlite"

The Bedrock case is the one that can mis-bill rather than just miss: parse_bedrock_offer writes with table.setdefault(key, {})[direction] = price, so two AWS products differing only by a trailing 3-digit marker collapse onto one key and the later price wins — cross-model mispricing rather than a clean miss. In lookup_cloudflare_workers_ai the stripped candidate can match a different catalog entry.

Good news: mistral-small-2501 is safely untouched (4 digits), so the Mistral snapshot ids aren't at risk. Either run the same verification against Cloudflare's catalog and the AWS offer names, or scope the new arm to the OpenRouter lookup that motivated it.

# alias on every streaming call.
last_with_usage = {
"usage_metadata": payload["usage_metadata"],
"model_version": payload.get("model_version"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gemini samples model_version only from the chunk that carries usage; Anthropic's fix in the same commit persists it.

last_with_usage is rebuilt from scratch on every chunk where usage_metadata is truthy, and model_version is read from that same payload. So for chunks [{model_version: "gemini-2.5-flash-002", usage_metadata: {…}}, {usage_metadata: {…}}] — a usage-bearing final chunk that omits the version — the emitted event reverts to model_version=None, resolve_model falls back to the requested gemini-flash-latest, and price mode misses exactly as it did before the fix.

The Anthropic wrapper handles the identical problem correctly, with resolved_model = _merge_stream_usage(...) or resolved_model carried across all events. Same treatment here would do it. The new test can't catch it because FakeModels puts model_version on every chunk, and the async path at line 136 has the same code.

Tiny related thing in wrappers/anthropic.py:107: the docstring says "The caller keeps the first one it sees", but x = f(...) or x keeps the last. Only message_start carries message.model today so it's latent — but the docstring is the only statement of the rule. resolved_model = resolved_model or _merge_stream_usage(...) would match the doc and short-circuit the merge.

if not isinstance(payload, dict):
return None
usage = payload.get("usage")
if isinstance(usage, dict) and usage:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carrying the gateway's echoed model through makes _infer_provider depend on what the gateway echoes — worth confirming with a captured /compat streaming chunk.

Fixing the streaming model drop is right, and belt-and-braces with the lookup_cloudflare_workers_ai prefix strip. But it does change the input _infer_provider sees on the streaming path, and it makes one sentence of that function's new docstring false: "what a streaming call always reports (the synthetic usage payload carries no model, so resolve_model falls back to the requested string verbatim)". After this commit the payload does carry a model, sourced from the gateway.

That matters because _infer_provider only returns workers-ai for @cf/… or workers-ai/@cf/…. If Cloudflare's /compat streaming chunks echo model as anything else — a bare slug, a normalised name — a streamed Workers AI call is stamped openai, looked up against OpenRouter, and degrades to token events. Which is the regression the workers-ai fix closes, reachable again on the streaming path only.

It may well echo the prefixed form, in which case there's nothing to do but correct the docstring. One captured streaming chunk from /compat settles it, and it'd be worth a fixture either way — this is the one path where the two halves of the fix could disagree.

convention. The JS port already defaulted to zero here, so the two repos were
differently wrong on the same input.
"""
base = _parse_price(usd)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply_markup now returns "0" on bad input with no report — a silent $0 bill where there used to be a loud failure.

Not raising from a money helper is the right call, and JS parity is a good reason. The gap is that nothing reports it. Verified: apply_markup("abc","1.2"), apply_markup("1.5","abc"), apply_markup("1.5","-2") and apply_markup("1e16","1") all return "0", and in _push_cost_event that flows straight into "value" and money_str_to_cents(...)precise_total_amount_cents="0". A split cost event is billed at $0.00 with no log line and no on_error.

Worse, it consumes the transaction_id: a corrected re-run of that window is then rejected as a duplicate, so the $0 sticks. That's the same failure shape I flagged on the Databricks PR for negative usage_quantity.

It's a defensive path — parts["cost"] comes from compute_cost, which produces well-formed money strings, so I can't construct a live trigger. But this commit's own theme is "report every dropped event", and this is the one new silent-zero path. A _report_error alongside the zero would make it consistent with the overflow and no-subscription reports. Note _parse_price also rejects ≥1e16, so apply_markup now zeroes some inputs the old code computed correctly.

# 50 of the 64 available.
if len(batch) < _CF_PER_PAGE:
break
if page >= _CF_MAX_PAGES:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 40-page bound still allows ~400s of blocked event delivery per refresh.

_CF_MAX_PAGES = 40 sequential requests.get calls at the default 10s request_timeout_seconds is up to ~400s, all on the lago-queue thread inside maybe_refresh() — which _run calls before _take_batch(). A slow endpoint, or one that ignores page and keeps returning full pages, stalls every queued billable event for that whole walk; and since _cloudflare_stale clears only on success, the walk repeats.

The changed comment says the bound exists so the loop "must not stall event delivery indefinitely" — which is true, it's now bounded. But 400s is a stall, just a finite one.

This is the same family as the prime()/TTL item you're deferring, so it probably belongs in that same follow-up rather than here. Two cheap options for whenever it lands: drain the buffer before maybe_refresh() in _run, or bound the whole walk by wall clock the way the shutdown drain already does with min(self._max_retry_seconds, 10.0), instead of by page count.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants