Fix the queue, subscription and pricing paths flagged in review - #18
Merged
anassg-lago merged 1 commit intoAug 20, 2026
Merged
Conversation
Each of these was a fix that made something else worse, so each carries a
test that fails when the fix is reverted.
Queue overflow could deadlock the customer's thread. _lock is a plain Lock,
so raising the new overflow report while holding it deadlocked any on_error
hook that itself emits — a plausible hook, since reporting a billing gap by
emitting a metric is an obvious thing to do. The report now runs with the
lock released, plus a threading.local guard bounding re-entrancy to one
report per overflow per thread (reporting after release alone recurses,
because the buffer is still full when the hook runs). Covered by a subprocess
test: an in-process deadlock wedges interpreter shutdown and hangs the whole
suite rather than failing it.
api_url="" stopped all billing. Guarding explicit args on "was it passed?"
meant api_url=os.environ.get("LAGO_API_URL", "") with the var unset stored "",
and an empty base URL is unrecoverable downstream: requests raises
MissingSchema, which is not a LagoApiError, so the queue reads it as
transient and retries at the 60s ceiling forever. api_url is now guarded on
non-emptiness, so "" falls through to config and then to the production
default, exactly as an omitted argument does.
workers-ai was missing from _OUTPUT_INCLUDES_REASONING, the output-side twin
of the cache-read entry added for it. The /compat endpoint returns the OpenAI
shape, where reasoning is a subset of output, so counting it additively
inflated the basis — 1900 for a call that consumed 1100. Unreachable today
(Workers AI reports no reasoning tokens at all, verified live), but the two
sets must agree about the same provider for the same reason, and the
cache-read side IS live-reachable.
413/402/415 head-of-line blocked the whole FIFO. These reject a batch AS a
batch, so re-sending it can never succeed — treating them as transient
re-prepended the identical batch and backed off to 60s indefinitely.
Classifying them permanent routes them to _send_individually, which splits
the batch and delivers what is deliverable: the case that path was built for
and could not previously reach. 405/410 stay transient.
Also de-flakes test_overflow_drops_oldest_at_exact_boundary, which set
max_batch_size == max_buffer_size and so let the worker race the assertions
for the lock. The overflow fix widened that window enough to fail 5 runs in
6; the fix is the one its sibling test already carries.
---
A moving "~" alias could overwrite a real listing's price, decided purely by
catalog order. Stripping OpenRouter's "~" marker is what makes a plain
"-latest" id priceable at all, but it wrote the alias-derived keys with plain
assignment. Collision-freedom was verified against the live catalog and still
holds — that was a property of the day's response, not of the code. On a
synthetic pair the same lookup returned 0.009 or 0.001 depending only on
position in the response. Alias keys are now written only when absent, so a
real listing always wins; the "~"-spelled id still resolves to its own entry,
and non-alias entries keep plain assignment.
The -\d{3} strip arm is now scoped to OpenRouter, the only source that ever
needed it (Gemini's "-002" revision, which OpenRouter omits). The shared
helper also builds the AWS/Bedrock price keys, and there a shortened key does
not merely miss: bedrock_model_key feeds table.setdefault(key, {}) per
direction, so two models collapsing onto one key silently overwrite each
other's rate. All four live catalogs are clean (OpenRouter 415 ids, Cloudflare
64, AWS offer 77, captured Bedrock 39), so this was latent — the split makes
it structurally impossible rather than empirically absent.
apply_markup's two bad-input fallbacks are not equivalent, and the ports
disagreed on one. An unparseable cost means there is nothing to bill, so 0 is
right; an unparseable markup means only the multiplier is unusable, and
returning 0 there discards a good cost. Python returned "0" for both, JS fell
back to 1.0 for a bad markup, so identical input would have billed
differently. Python now matches JS.
That last one is defence in depth, not a live fix, and the code says so:
every emit path already runs the customer's markup through coerce_markup
(which falls back to 1.0 and reports), and both arguments are _fmt_money
output by then, so neither can actually arrive unparseable. What had no
coverage was the end-to-end consequence of that guard, now pinned: a customer
sending markup="1,5" gets the cost billed at 1.0 rather than zeroed, and the
lost markup reaches on_error.
---
Five small things, each with a test that fails when reverted.
A recovery path silently reversed FIFO order. _send_individually re-queued
each transiently-failing event as it went, and _replay_failed PREPENDS — so a
413 batch of a,b,c,d,e whose b,c,d failed while isolated came back as d,c,b.
FIFO is the queue's contract: it is what makes the oldest-dropped-first
overflow policy and Lago's own event ordering mean anything. Survivors are now
collected and re-queued once.
A negative token count was dropped without a word. nonzero_numeric correctly
filters it (Lago would otherwise sum a negative billable quantity), but this
was the last drop path that never reached on_error — the same gap already
closed for queue overflow and for an unresolvable subscription. It is
reachable, not theoretical: CanonicalUsage is exported and emit() takes one
directly, which is the documented way to backfill usage the SDK did not
intercept. Reported before the empty-check, so an event whose only fields were
negative still reports instead of returning silently.
One log line per dropped event, not two. _report_error already invokes
on_error AND logs; an extra logger.error beside it emitted the same drop twice
at two levels, so a customer grepping logs counted one lost call as two.
verify_ssl=False could crash LagoSDK() construction. The warning suppression
reached through requests.packages, a legacy compatibility alias with no
guarantee of existing, in an unguarded attribute chain inside __init__. Now
imports urllib3 directly, wrapped: suppressing a warning must never fail
construction. This sits on an advertised path — verify_ssl is a first-class
constructor argument the docstring recommends for local dev — so the crash
would have hit exactly the setup the flag was added to serve.
WORKERS_AI_COMPAT_PREFIX had drifted into two definitions.
adapters/openai_native decides the provider from it and pricing strips it
before a catalog lookup; those two must never import each other, so it now
lives in canonical (which imports nothing from the package — no cycle either
way, and no pulling pricing's ~50KB into a lightweight adapter). A drift
between the copies would have been a silently unpriced call rather than a
crash, so a test asserts there is exactly one definition in the tree.
ancorcruz
reviewed
Aug 20, 2026
ancorcruz
left a comment
ancorcruz
left a comment
There was a problem hiding this comment.
Review — verified, no blocking findings from me
All four blockers from the previous round are fixed, each with a regression test that pins the specific failure. I verified the code rather than the commit message in every case:
| Item | Fix | Test |
|---|---|---|
on_error inside the lock |
Report moved out, should_wake shape |
test_overflow_report_does_not_deadlock_a_reentrant_callback |
api_url="" |
if api_url: |
test_empty_or_absent_api_url_keeps_the_production_default |
workers-ai reasoning double-count |
Added to _OUTPUT_INCLUDES_REASONING |
test_openai_shaped_providers_treat_reasoning_as_a_subset |
413 head-of-line blocking |
402/413/415 → permanent → _send_individually |
test_batch_only_4xx_is_split_not_head_of_line_blocked |
Plus the ~-alias setdefault, the -\d{3} scoping, WORKERS_AI_COMPAT_PREFIX deduplicated into canonical.py, the double-log, negative_numeric(), and urllib3 imported directly.
Three places you improved on what I proposed, worth recording:
-\d{3}scoping. I suggested verifying the arm against the other catalogs. You did that and then split it into_OPENROUTER_VERSION_SUFFIXso the Bedrock key-builder can't reach it at all. Structurally zero beats empirically zero — and thesetdefaultmis-pricing you point to inbedrock_model_keyis the right reason.apply_markup. I asked for a report; you made the better distinction instead — unparseableusdmeans there's nothing to bill (0), unparseablemarkupmeans only the multiplier is unusable (1.0, bill the real cost). I accept the argument that both are unreachable givencoerce_markupruns first and the cost strings are_fmt_moneyoutput, so a report would fire for nothing. Dropping that request.- The re-entrancy guard. I only reported the deadlock. The second-order problem — hook runs, buffer is full again, hook pushes, overflows, re-enters unboundedly — I missed entirely. The per-thread
threading.local()flag is the right shape, and running the check in a subprocess with a timeout is the right way to test it; an in-process assertion would have hung the suite instead of failing it.
And one bug you found that I didn't: _replay_failed prepends, so the per-event call inside the isolation loop inverted the survivors (b,c,d → d,c,b). I checked extendleft(reversed(batch)) and the assertion on ["b","c","d"] — order is preserved now. That one was invisible from the outside and it mattered.
Judgment calls I'd leave as they are: 405/410 staying transient is defensible for the reason given (a deploy can fix a misroute; dropping loses events). urllib3.disable_warnings() is still a process-global filter rather than per-client — the second half of my original note — but there's no clean per-request alternative and the customer has explicitly opted out, so I'd let it stand.
CI has not run on this branch (see #17 — ci.yml filters on base main, so nothing in this stack triggers it). I ran the job locally: ruff, format, mypy clean; 531 unit tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Answers eight of your round-two threads. Every fix has a test that fails when the fix is reverted — that's the check I ran on each one, not just "tests pass".
The four regressions the last pass introduced
on_error. Reporting after release alone still recurses (the buffer is full when the hook runs), so there's also athreading.localguard. The test is a subprocess: in-process, the deadlock wedges interpreter shutdown and hangs the whole suite instead of failing it.api_url=""— confirmed, and worse than "the default is lost":MissingSchemaisn't aLagoApiError, so the queue treats it as transient and retries at the 60s ceiling forever. All billing stops with a growing buffer as the only symptom.workers-aimissing from_OUTPUT_INCLUDES_REASONING— fixed. One correction: it is not reachable today. Workers AI reports no reasoning tokens at all (verified live against three reasoning-capable models and the gateway's Logs API), so the 1900-vs-1100 figure is arithmetic, not a bill anyone paid. Fixed anyway because the cache-read side is live-reachable and the two sets must agree about the same provider._send_individually, which splits the batch — the case that path was built for and couldn't reach.Pricing
~-alias overwrites a real listing — confirmed order-dependent (0.009 vs 0.001 on a synthetic pair). Alias keys are now written only when absent.-\d{3}governs Bedrock and Cloudflare too — scoped to OpenRouter. Your point about the safety check was right: all four live catalogs are clean, so this was latent, but on Bedrock a shortened key silently mis-prices rather than missing.apply_markupreturns"0"silently — your premise doesn't hold, so I didn't add the report.coerce_markupalready guards everyemit()path (falls back to 1.0 and reports), and both arguments are_fmt_moneyoutput by then, so the branch is unreachable;apply_markupisn't exported either. I wrote the reporting, saw its end-to-end test fail withgot ['pricing'], and removed it as dead code. What I kept is a real divergence you didn't name: given a valid cost and a bad markup, Python returned"0"(destroying the bill) where JS returned the un-marked-up cost. Python now matches JS, and the guard's end-to-end behaviour — which had no test — is now pinned.Also in here (from the review body rather than a thread): the FIFO reversal in the isolation path — collected and re-queued once, since
_replay_failedprepends; the negative-count drop now reacheson_error; one log line per drop instead of two;verify_ssl=Falseno longer risks crashingLagoSDK()(requests.packagesis a legacy shim); andWORKERS_AI_COMPAT_PREFIXde-duplicated intocanonical.Green at this commit: ruff +
ruff format --check+ mypy clean, 531 unit tests. Also live-tested end to end against a real Lago across all seven provider paths in both pricing modes.