The minimax wire lost its tier value on the assumption that these models
reason by default, so injecting nothing still reads as "on". T10 measured
the real gateway and the assumption does not hold: MiniMax-M3 with no
reasoning parameter did not reason in 5 of 5 rounds, while all six
strength values worked. Existing downstreams on ENABLE_THINKING=true
went from reasoning to silently not reasoning, and the capability table
cannot catch it because phase 5 lets auto through unconditionally.
Restore on_base to the old {"reasoning_effort": "medium"} verbatim. This
is a stopgap - it hands the tier choice back to the library, which this
work set out to remove. The real fix is to constrain auto by the
capability table, a public behaviour change tracked as issue #21.
The assertions that said "minimax injects no tier on the on-tier" go
back with it; each carries a note on why it moved twice.
`test_extra_body_overrides_the_profile_slot` guards that a source's
`extra_body` wins over the slot the profile injects. Since T5 made
`enable_thinking=True` mean `auto`, its minimax fixture injected an empty
fragment — swapping the two update calls changed nothing and the whole
suite stayed green, while a real source (zhipu, glm-5.3, tier `low`)
would have shipped `low` instead of the `high` the caller wrote. Move the
fixture onto that combination, where the two sides write the same key.
`emit_attempt`'s docstring promises `reasoning_applies` carries no
default so that a missed argument is a TypeError rather than a silent
`True`; nothing enforced it. Pin the signature the way `record_llm_call`
is already pinned.
`resolve_thinking` is exported, and its third argument turned from `bool`
into `Effort` this cycle — so the most natural downstream call now passes
the `"low"` it read out of JSON or config. Every gate inside compares by
identity, and `"none" is Effort.NONE` is always false: the tier was not
rejected, it was silently misjudged. Phase 2 read the on-form for a
request that wanted the off-form, Phase 4 was skipped entirely on a model
that cannot be disabled, and the only visible symptom arrived much later
as an AttributeError from `.value` — an exception that is neither
documented nor one of the four error classes.
Design 4.4 already lists this as the fourth entry a tier can come in
through; this makes the code agree with it.
Twenty-five columns and not one of them answered "which tier was this?",
so the question the whole issue exists to settle - does a higher tier buy
anything - had no way to group its data.
The three emit entry points deliberately disagree, the way sampling
already does. A successful attempt records what the transport actually
sent: with EFFORT_FALLBACK=nearest a request for medium goes out as low,
and recomputing here would file the row under a tier that never left the
process. A failed attempt has no response to read, so it falls back to
the requested tier - which is exactly right for the tier errors that are
rejected before any HTTP happens, because the rejected tier is the
signal. Cache hits and terminal failures have no chosen source at all,
so a source-level tier is not a thing they could report.
emit_attempt now demands to be told whether the path reasons at all.
Embedding and OCR share the emitter but never send reasoning parameters;
without the flag a source that mistakenly carries ENABLE_THINKING would
hang a tier on a call that could not possibly have run at one.
The value lands as a plain str. StrEnum is a str subclass and asyncpg
promises nothing about encoding subclasses, and a telemetry write that
fails is only a warning - Postgres would just quietly lose the column.
NULL means nobody declared a tier, which is not the same statement as
'none', and the two must never be folded together.
Adding applied_effort to LLMResponse put it through the cache round
trip, where JSON stores a StrEnum as its plain value. Rehydrated raw, a
hit would hand downstream a str while the annotation says Effort, and
every `is Effort.LOW` in the library would quietly answer False on the
hit path only -- the same trap thinking_observation already has a
coercion for.
A value outside this version's vocabulary degrades to None rather than
failing the entry: projects sharing one Redis would otherwise keep
invalidating each other's writes over an attribution field, and None is
the honest reading of a tier this version cannot name.
The transport now hands back the tier it actually sent, and that tier
rides TransportResult into LLMResponse. It is not the requested one:
under EFFORT_FALLBACK=nearest a medium request goes out as low, and
telemetry grouping by the requested tier would file the row under a tier
that never left the process.
Reconciliation judges the same tier instead of the old enable_thinking
bool, and the warning throttle keys on it. Keyed on the bool, every tier
of one model shared a single key, so the second contradiction was
silenced for the lifetime of the transport. The predicate is an identity
check against Effort.NONE on purpose -- the member's value is the
non-empty string "none", so any truthiness test would send every strength
tier down the "asked to disable" branch and invert the alarm.
The absent-tier side has had goldens since issue #4; the side that
actually carries a tier had none, so swapping str() for repr() in the
cache key changed the literal without turning the suite red. Pin both
literals. Note the fingerprint's json.dumps emits the same bytes for an
Effort member and its .value, so that pair is indistinguishable by any
test -- the golden pins the literal itself instead.
Both were wired but unwitnessed: hardcoding the transport's fallback, or
blanking the source tier the assembly guard reads, left the whole unit
suite green. Cover them where the value is visible -- the bytes on the
wire for nearest-vs-error, and the assembly-time refusal that must name
low as the executable alternative.
A provider that registered a disable form but no enable form was told
its shape was unknown and pointed at register_provider -- work it had
already done -- for a request that only ever needed the disable form.
The old bool code took the slot by direction; the tiered rewrite lost
that. Take the relevant field again, and keep "shape unknown" for the
case where both halves are missing, so the "cannot disable" wording
still owns the half-missing case.
Both public assembly paths took the tier on trust: a bare "none" from
JSON or a hand-built SourceConfig stayed a str, and `is Effort.NONE`
then read it as a contradiction and crashed on `.value` while wording
the error -- the caller got an AttributeError where a ValueError was
promised, and on the request side that unclassified exception walked
straight through the transport's ThinkingUnsupportedError catch and the
retry classifier.
Normalize at the two entrances instead, matching what the .env path has
always done, and let EFFORT_FALLBACK be spelled with the same freedom as
its neighbour.
The per-call reasoning tier never reached the cache key, and the model
fingerprint could not stand in for it: the fingerprint is computed once at
assembly time, so two calls on the same client asking for low and max looked
identical to it. Same messages, different tiers, one shared entry -- the
verbatim replay of issue #4's five seeds all hitting the same response.
Source-level tiers join the fingerprint under the same rule enable_thinking
already follows (appended only when the source takes a position), and the
filter that decides which sources enter the mark set is widened to match --
without that, a source configured with nothing but REASONING_EFFORT would
never reach _fingerprint_mark at all.
None (no opinion) and Effort.NONE (asked not to reason) stay distinct keys.
Sources that opine on neither keep byte-identical keys and fingerprints, so
nothing existing cold-starts.
The Transport port took the request apart into five arguments, so a tier
placed on ChatRequest could never reach _build_payload: the field was set,
read by nobody, and silently ignored - the exact shape of failure that sent
downstream to extra_body in the first place.
complete() now takes reasoning_effort with no default, matching the
TelemetryRecorder convention: a default would turn a missing hand-off into
a silent 'no opinion'. All four fakes move with it, since @runtime_checkable
checks method names and not signatures.
EmbeddingTransport and OcrTransport are deliberately left alone - they have
no reasoning semantics - and a test now holds that line.
_build_payload drops its inline sugar conversion for effective_effort(), so
the guard and the hot path share one judgement, and passes the source's
effort_fallback for the same reason.
The three-layer priority (call > source > enable_thinking sugar > silence)
now lives in one pure function, thinking.effective_effort(). The assembly
guard and the request hot path used to each carry their own inline copy of
the sugar conversion; two copies of the same judgement drift into the worst
shape there is - passes at assembly, raises at runtime.
The guard now also honours effort_fallback, so a source that opted into
nearest is no longer sentenced at assembly for a tier it could have mapped.
resolve_thinking now takes an Effort instead of a tri-state bool, and the
four gates become five. The new one sits ahead of the generic tier check
on purpose: asking for `none` on GLM-5.3 used to fall through to "none is
not supported, pick low/high/max", which loses both the fact that the
model cannot stop reasoning and the one tier the caller could switch to
right now. Without that alternative, downstream goes looking for
extra_body — which is how issue #20 happened in the first place.
The return type is a ThinkingResolution rather than the payload alone.
Under fallback="nearest" the tier that goes out is not the tier that was
asked for, and telemetry has to record the one that ran, or task 10 files
a call under a tier it never used. Ties in that mapping go to the weaker
side: a silent medium -> max is a multiple of the bill, and the library
does not raise a caller's price on its own.
Two readings the design left implicit, both settled the way its own
compatibility promise requires:
- `auto` is exempt from the tier list. It means "on, no tier named",
which in the body is the absence of the effort key, not a value of it.
Checking it against the list would break every existing source that
sets ENABLE_THINKING=true against deepseek-v4 or glm-5.3.
- `none` is never a mapping target. Turning "think less" into "do not
think" reverses the decision instead of cheapening it; a switch-only
model maps to `auto` and a model that only has `none` still errors.
Both call sites convert enable_thinking in place for now; task 5 folds
that into effective_effort along with the source- and call-level tiers.
The previous commit added the name to __all__ but never bound it, so
`from polygateway import ThinkingWire` and `import *` both raised while
the whole suite stayed green — the export test names symbols one by one,
and nobody thought to add the new one.
The guard is now the invariant rather than a longer list: every name in
__all__ must be an attribute of the package.
Eight segments now, and each one holds a ThinkingWire instead of two
fixed fragments: off, on_base, and the key a tier gets written to. The
two fragments could not say "on, at this depth", which is what every
current generation model wants.
Two deliberate behaviour changes fall out of it. The openai segment stops
reporting its shape as unknown — reasoning_effort is OpenAI's own field,
not a vendor dialect, so a compatible endpoint behind the gateway takes
it. And minimax's on-tier stops carrying a hardcoded medium: that was the
library picking a price for the caller, and medium is not even a tier GLM,
kimi or deepseek serve.
The issue #5 guards stay; their sample moves from "the openai segment" to
an explicitly registered unknown one, which is what they always meant to test.
The boolean could say a model reasons or does not. It could not say what
GLM-5.3 and Gemini 3 Pro actually do: refuse to stop reasoning while
still letting you ask for less. So capability becomes the list of tiers a
model serves, and `none`'s presence in it is what "can_disable" now reads.
Effort carries `auto` alongside the strength tiers. Nine of the models on
our gateway are pure switches with no tier to name, and without `auto`
they would have to borrow a strength tier to mean "on" — which is the
exact bug this work exists to remove.
Tiers land as documented guesses from four registries that agree; every
entry says so in its evidence, and task 10 replaces them with measurements.
Both reviews landed on the same line independently. _as_role swaps the
credentials in the DSN with a regex, and when the pattern does not match
it returned the string unchanged. Two shapes miss it: no inline
credentials, and a unix socket URL. Either one is a legal DSN.
What that costs is not a broken test. The sandbox builds, every
assertion still passes, and bare_dsn is now the admin connection, so the
worst-case case runs the real script with --apply as a superuser against
the shared table. The verifier ran that command as a dry run to see what
it would have done: target public.llm_calls, 11 rows to delete. The case
would still have gone red on the exit code, after the rows were gone.
It raises now. There is also a second check that connects and compares
current_user, because a successful string substitution is not the same
as connecting as that role -- PGUSER and friends still override. The
whole design rests on that connection having no grant on the shared
table; a string comparison is too thin a thing to rest it on.
That check has to stay inside the try. Past it the cleanup statements
have already been merged into the fixture-level stack, and unwinding
again runs DROP OWNED BY twice, which has no IF EXISTS.
The catalog probe took any SQL and ran it on the admin connection. The
design claims withholding the DSN makes the boundary structural; that
was only true of the connection string, not of the capability. It takes
SELECT now.
--table's schema half is restricted to plain identifiers. Not a
security fix, since the name goes through a parameter and _quote: the
help text says complex identifiers are unsupported and the code was
accepting them anyway.
Seven cases wrote straight into the shared table and told their rows
apart by a call_id prefix. Reading was never the problem; the prefix
did that correctly, and it was built for concurrent runs. What it could
not do was stop those writes and deletes from moving a row count that
another test was watching, which is how issue #18 turned red.
They now write into sandbox schemas, which also ends the orphan rows a
killed run used to leave in there. Six fixtures collapse into factory
calls; what they yield is unchanged, so the cases that consume them did
not have to be touched, which is what makes them worth anything as a
check on the move.
Two of the seven kept something. The pool footprint case needs a unique
application_name, since connections are an instance-wide resource that
schema isolation does not reach, so it generates its own uuid instead
of borrowing the run prefix. And the frozen-columns case was querying
information_schema without a schema filter, so any leftover table of
the same name anywhere in the database could fail it: the file already
knew this, in a comment explaining why another fixture cleans up so
carefully. It now filters, and gets checked against a leftover table
planted on purpose.
The gate that keeps the literal out of tests/ is a smoke alarm, not
proof. Concatenation and parameterised queries walk straight past it.
The isolation is the factory withholding the admin connection and the
script running as a role with no grant.
Until now the target came from whatever search_path resolved to. The
script printed what it found, but that print and the DELETE happen in
the same run with nobody in between, so it only ever helped the person
who ran a dry-run first. Swap the role that runs it and "$user" can
resolve somewhere else entirely.
--table takes the whole qualified name and resolves it directly. The
table half has to be llm_calls: a version that accepts any name turns
one typo into a general purpose row deleter, and any table with a
created_at and a tenant_id would go through the same batched DELETE
without complaint.
The tests that run it now run as a role that owns its own scratch table
and holds nothing on the shared one, so the row-count snapshot could
go. What replaced it is a case that lets the script fall through to the
shared table on purpose and asserts it exits 2 having deleted nothing.
That one has no red-first path, since making it red means running it as
the superuser, which is the thing being prevented; the finding's probe
covers it instead.
Five of the new usage tests passed before the flag existed, because
argparse rejects an unknown --table with exit 1 and the word --table in
stderr, which is exactly what they asserted. They now also assert the
error is not "unrecognized", which is the difference between testing
the validation and testing argparse.
Seven copies of "create a schema, hang it off search_path, drop it in
teardown" were spread across two files, each with its own cleanup. Any
one of them written wrong leaves the residue on a database shared with
real batch runs. This is one implementation, and it makes "the test
cannot reach the admin connection" a structural fact rather than a note
in a docstring.
Three role modes cover every fixture that exists today: none for plain
schema isolation, owner for the retention script's own runs, grantee
for the least-privilege deployment cases. Owner runs its DDL as itself
so it ends up owning the table; grantee is the opposite, since that
case only means anything when someone else built it.
The schema and the role deliberately get different prefixes. Give them
the same name and "$user" resolves to the sandbox, which hides the
shared table and quietly turns the worst-case test into a test of
nothing.
Writing it also turned up a bug in my first version: rolling back a
failed sandbox unwound the whole stack, so an earlier sandbox in the
same test lost its role mid-use. The test for it fails with a password
authentication error, which is what that looks like from the outside.
Each call now unwinds only what it created, and cleanup tries every
statement before raising, since one failure stranding the rest means
global roles left behind by hand.
The reasoning_tokens docstring was still teaching downstream to treat
None or 0 as no reasoning. The changelog and the schema page had both
been corrected; the docstring had not, and it is the copy that ships in
the wheel and shows up on hover. Someone writing a report from it would
have counted every real MiniMax reasoning call as not reasoning, which
is issue #16 all over again with the tests green.
The original wording stays, since reading pre-1.3.1 rows still needs
it. What follows it now says when it expired and what to read instead.
Two more places had drifted the same way: the changelog and the
architecture doc described the throttle and the cache fallback as they
were before this review, which is to say as the opposite of what the
code now does.
The claim that the two throttle sets would suppress each other does not
survive checking, as the mutation testing showed: their key spaces do
not overlap. Keeping them apart is still right, but for the honest
reason, which is that the two warnings have unrelated lifetimes.
Three of them were the same shape as the bug this branch exists to fix:
something goes wrong, the library swallows it, and the caller is left
with a number that means the opposite of what happened.
The throttle key had no source in it. Five sources on one model is the
normal case here, so the first one to break would warn once and silence
the other four for the life of the process, and the message never said
which gateway to look at.
An unknown verdict in a cached entry threw away the whole response. The
rehydrator tolerates unknown fields but not unknown values of a known
field, so two library versions sharing a Redis would each invalidate
the other's entries: halved hit rate, and the only log line says the
cache rebuild failed. A purely observational field should not be able
to void a response whose content is intact.
Normalising for telemetry now degrades instead of raising, both for a
bare string and for a value outside the domain. Either one used to
reach the same except and cost the whole row, which is exactly how
1.3.0 lost nineteen calls without anyone noticing.
The telemetry field count is taken from inspect.signature, not from
memory, because that is the one the release checklist keeps catching.
llm-calls.md said 22 and was two rounds stale; fixing the title alone
would have left the table contradicting it, so tenant_id and meta are
documented too.
The production template needed no new column — it derives them with
LIKE. What it gained is an assertion that it must keep deriving them
and must not inline a column name, which is the drift that could
actually happen.
The changelog leads with the three breaking items. A patch number
carries no warning by design, so the entry has to.
The four cases were red because the criterion could not see the
evidence. reasoning_tokens has been None on this route ever since
MiniMax stopped reporting completion_tokens_details, while the same
call carried 185 characters of reasoning prose the assertions never
looked at.
L5 asserted something that cannot happen. M3 returns neither prose nor
usage detail over the plain endpoint, so demanding that the
non-streaming path observe reasoning could never pass. It now asserts
what is true and worth holding: the prompt_tokens anchor still
separates the two directions, so the parameter did reach the model, and
the verdict is not ABSENT, so the library marked the gap honestly
instead of dressing it up as no reasoning.
_ON_MIN_COMPLETION is gone. The two directions overlap in output length
— 46 at most disabled, 13 at least enabled — so that fallback drew a
line through noise and only made the criterion look defended.
This issue surfaced only because someone ran a slow suite that is
excluded by default and had not been run for eighteen days. As a column
it becomes a query: which model stopped being observable, and when.
The emitter unwraps the enum to a plain str at the single _record exit.
asyncpg makes no promise about encoding a str subclass, and a telemetry
write that fails is downgraded to one warning — it would not crash, it
would just quietly cost the Postgres path a column. Normalising at the
emitter follows what tenant_id, meta and sampling already do.
The column is appended last in COLUMNS and in both DDLs. An existing
table can only take ALTER at the end, so putting it anywhere else
forks the physical column order between a freshly built database and a
backfilled one.
asdict keeps the enum and json.dumps writes it as a string because
StrEnum is a str subclass, but nothing turns it back on the way in, so
a cache hit returned a plain str where the annotation promised an enum.
Verified end to end rather than assumed from the subclass relation.
A value outside the domain now raises inside the existing guard and the
call falls back to source, which is the right direction for a poisoned
or stale cache entry. Entries written before this column existed still
replay: the guard checks for the key first, and a test pins that, since
turning it into an unconditional conversion would quietly turn every
pre-upgrade entry into a permanent miss.
The M3 evidence sat at 08-02 for twenty-three days while nobody could
tell whether it still held. A declaration that goes stale in silence is
the failure this issue is really about, so the library now compares
what it declared against what it just observed and says so when the two
part ways.
Judgement is separated from logging: reconcile_thinking returns the
warning text, so tests assert on the text instead of parsing logs.
Two cases that look alike are kept apart — a model whose capability is
registered gets a drift warning quoting its evidence, an unregistered
one is never told the table said anything, because it never did.
False x UNKNOWN stays silent on purpose. UNKNOWN cannot falsify
anything, and warning on it would fire on every disabled call M3 makes
over the plain endpoint. A warning that always fires is not a warning.
Both assembly paths fill it, streaming and non-streaming alike. Filling
only one is exactly the divergence this issue exposed: M3 returns
reasoning prose over SSE and nothing at all over the plain endpoint, so
a verdict computed on one path says nothing about the other.
The field defaults to UNKNOWN on both TransportResult and LLMResponse.
A transport that does not judge should not get to declare absence on
the provider's behalf, and a default that stays silent is the only one
that cannot lie.
providers.py had been holding two jobs: the registry of what each
provider looks like, and the decisions made from those declarations.
Adding response-side judgement would have made it the module for
everything about reasoning, so the decisions move to thinking.py and
the registry keeps only profiles and their lookup.
Moving a module breaks any deep-path import of what moved, so the six
public symbols are promoted to the package root at the same time. The
top level is this library's stated API surface; giving downstream a
stable name to import is what makes the next reorganisation harmless.
observe_thinking stays unexported — downstream reads the verdict off
LLMResponse, and exporting it would be a permanent promise for nothing.
reasoning_tokens=None has been carrying two meanings at once, no
reasoning and no report, and the library resolved the ambiguity by
quietly claiming the first. ThinkingObservation splits them: UNKNOWN
says the call left no signal, ABSENT says the provider reported zero.
The verdict ranks evidence by hardness. Reasoning prose is the fact
itself; reasoning_tokens is a report about the fact, so a missing
report cannot overrule prose that is right there. The prose check
strips first, since a gateway that returns whitespace is not evidence.
The enum lives in types.py, not in the new thinking.py, because
LLMResponse is typed on it and the innermost layer must not import a
decision module.
client.telemetry_status exists so downstream can reconcile telemetry
programmatically, but annotating its return type meant reaching into
polygateway.types while the convention here is that the top-level
exports are the public API surface. The port itself stays unexported:
nobody outside the library implements it.
The status snapshot reports elapsed time, so asserting retry_after_s
against the real monotonic clock was really asserting that a few lines
of code take zero time; it failed at 59.99993 vs 60.0. The recorder
already accepts an injected clock for exactly this reason.
The min_size=10 default survived to 1.2.4 because every PG test injected a
pool and thus skipped the pool-building path entirely. Unit tests now assert
the create_pool arguments, but "we passed min_size=0" and "the server really
opened that many backends" are two different claims, and only a real instance
can settle the second one. Count via a run-unique application_name carried on
the DSN: the instance is shared with other projects, so counting by database
or role would fold their connections into ours and make the case flaky by
construction.
Degradation is exercised through an unreachable DSN rather than by exhausting
the shared instance's connections. A refused connection lands in the same
class as exhaustion, and the fake clock lets the 60s cooldown be observed
without sleeping. retry_after_s is the signal that separates a real retry
(which renews the window) from the cheap short circuit (which does not).
Evidence: with create_pool reverted to its pre-fix form both cases go red
(observed 10 backends after a single write, and refusal surfacing at pool
creation instead of at prepare time).
The pool exhaustion in issue #15 was fatal only because min_size=10 forced
a transient error to surface at pool creation, and that step was hardcoded
to permanent death. Step is the wrong axis: it conflates "the DSN cannot
be parsed" with "someone else holds all the connections right now".
Failures are now classified by two rules. Fatal means the cause lies
entirely inside this process and cannot change, which only the
construction-time DSN satisfies. Everything else splits on whether the
failure has anything to do with this row's data: row-level failures drop
one row and keep trying, environment-level failures cool down for 60s and
then get exactly one retry, so a restarted database or a DBA creating the
table heals on its own.
42703 (missing column) is the single named exception and stays row-level
even though every row fails alike: issue #13 promised that the manual mode
trims the INSERT and exposes drift per row, and that promise outranks the
rule. Any future exception owes the same argument.
The _failed boolean is gone; the tracker is the only degradation state,
because two copies of the same fact drift apart. Closing stays outside
that state: it is the caller's own decision, not an anomaly to recover
from, so the snapshot reports it through dropped_rows and the drop reason
instead of raising the degraded flag on every clean shutdown.
Closing was the last unbounded wait on the shutdown path: asyncpg's
Pool.close() awaits wait_until_released() on every holder, so a single
in-flight connection parks the caller forever (60s only buys a warning).
It now runs under asyncio.wait_for and terminates the pool on timeout;
external cancellation still propagates untouched.
Closing is also final now. Clearing _pool used to leave the recorder free
to build a fresh pool on the next write - worse in the injected case,
where the owner believes it still holds every connection while the
recorder quietly opened its own. Recovery is a runtime concern (cooldown
retry), not a side effect of shutdown, so writes after aclose short out
and count the dropped row with a reason of their own.
Also covers the release/terminate fallback left untested by the pool
work: the fake pool needed for the close cases makes it nearly free.
The pool was the only external resource in the library that pre-allocated:
asyncpg's default min_size=10 turned pool creation into an all-or-nothing
action, so on a shared instance running low on connection budget the first
thing to fall over was the one component that must not fail silently
(4 clients x 10 = 40 idle connections just to write telemetry).
min_size=0 means "do not pre-connect" - asyncpg only builds holders - so
pool creation becomes free and never touches the database; connection
failures then land on acquire, the path that already drops one row and lets
the pool recover. max_size and the write budget become the library's
explicit statement about its own footprint, configurable through two new
keys whose defaults live in config alone (the recorder parameters are
required keyword-only, same discipline as auto_migrate).
The whole write - prepare, acquire, execute - now runs inside one
asyncio.timeout: acquire used to have no timeout at all, so a full pool
would hang forever on the caller's path. Release is explicit rather than
`async with`, because asyncpg shields release and reuses the acquire
timeout, which would let a single telemetry write consume twice the budget.
Telemetry degradation used to be a single warning and a private boolean.
In a long-running process that is indistinguishable from telemetry working:
issue #15 was only found by hand-reconciling milestone log lines against
llm_calls rows, after 19 calls had silently gone unrecorded. The SQLite
side was worse — once init failed, every write returned without even a
log line.
Degradation now has one shared owner. TelemetryStatusTracker holds the
state machine (enter/recover/drop/should-retry), announces entry and
recovery once each, and repeats the drop count under a row-and-time
double threshold so a degraded backend neither floods the log nor goes
quiet. Both recorders hold one; both count the rows they drop.
For programmatic consumers, TelemetryStatus is a frozen snapshot exposed
as telemetry_status on all three clients, resolved through a single
isinstance check. It is a separate optional port rather than a member of
TelemetryRecorder: that protocol is @runtime_checkable, so adding an
attribute would make every implementation that only defines
record_llm_call stop satisfying it — downstream isinstance assertions
would break on upgrade. The existing assertion in test_ports.py is what
keeps that decision honest.
Failure criteria are deliberately untouched here: Postgres still treats a
pool failure as permanent, only now visibly. `_failed` and the tracker
therefore both carry the verdict for the span of this one change; the
cooldown rework collapses them into the tracker alone.
A client used to close whatever transport, recorder or cache it happened
to hold, injected or not, so the first client to shut down killed the
backend its siblings were still using. That is why the explicit-sharing
path the architecture prescribes was unusable in practice and downstream
projects fell back to one private instance per client. The mirror image
of the same gap: the redis clients the factories build for the limiter
and the breaker were never closed at all, because nobody kept a
reference to them once they were handed to the retry middleware.
Ownership is now stated once, the way RedisLimiter already stated it:
whoever builds a resource closes it, injected ones are left alone. The
constructor is the full-injection path, so it owns nothing by default
and only the factories mark what they built. RedisCache gains the same
rule for its own client, and the three copies of the "probe for aclose,
fall back to close" dance collapse into a single helper so the next
correction cannot land in only one of them.
The pre-commit hook runs the whole suite, and tests/e2e/ talks to a real
LLM gateway, so whether a commit is allowed depended on how fast that
gateway happened to be. During the issue 14 work it blocked two commits
on two different cases; both passed when rerun alone, and the suite went
from 165s to 336s that hour.
The wasted minutes are not the real cost. Retrying on red teaches you to
read "test failed" as "gateway was slow", and a genuinely flaky bug then
gets retried away too. An alarm that cries wolf stops being an alarm.
test_thinking_live.py already carried the slow marker; the other three
files now match it, and the release checklist gains an explicit
`pytest -m slow` step so they still run where a human is watching --
without that step this change would just delete the coverage.
Also raises test_flat_legacy_keys_assemble's LLM_TIMEOUT from 120 to
300, matching .env. At 120 the case allowed half of what production
allows, on a gateway that needs the full 300 -- it measured 116s in a
solo run. The assertion is that the flat key name parses into
SourceConfig.timeout_s; the value itself was never under test.
Branch review caught the docs claiming something the code does not do.
CHANGELOG, README and the design's behaviour matrix all said a
force-opened source under circuit_open=wait waits out the full stall
window. It does not: the probe let through after each cooldown is a
real attempt, so it burns a max_attempts slot like any other, and a
401 source usually runs out of retry budget first -- reason is
retry_exhausted, not stalled. Which budget wins depends on
max_attempts against the cooldowns and the stall window.
The behaviour is right; only the prose was wrong. Charging the probe
to the retry budget is exactly the split issue #8 settled: the
question is who spends max_attempts, and a probe does send a real
request. A test now pins it so the claim cannot drift again.
Also drops the planned "woke up" log line. Each wait round already
logs on entry with its duration, and a still-blocked wake-up logs the
next round immediately, so a second line would only double the volume.
on_no_runnable now dispatches on why every source was rejected instead
of falling through two serial branches. Under wait, a fully open circuit
sleeps out the cooldown and comes back for another round; the breaker's
protection is untouched (still not a single request leaves during the
wait, so no quota or money burns) -- what changes is whether the caller
dies on the spot or queues.
Dispatching is not cosmetic. Left serial, wait would fall into the quota
branch and a caller with quota_full=fail_fast would get a
quota_exhausted error while its quota was in fact fine.
_nap sleeps to the cooldown deadline rather than polling every 10ms,
which for a 60s cooldown is 6000 round trips per in-flight call on the
Redis backend. Jitter is added on top instead of scaling the wait, since
waking early before a known deadline just earns another rejection. Both
arms clamp to the remaining stall budget, so the worst case per call is
stall_window plus one poll and does not drift with max_cooldown_s. The
clamp's lower bound is the jitter itself, not poll_interval -- the
latter would have lifted the existing [0.5p, 1.0p] quota polling.
Limiter rejections have always chosen between waiting and failing fast;
breaker rejections had no such choice. The new key is the missing cell
of that matrix, shaped exactly like QUOTA_FULL so there is nothing new
to learn. It defaults to fail_fast: flipping the default would move
every existing deployment's worst-case wall clock from milliseconds to
the stall window, which is the wrong direction to impose on anyone.
Single-source scopes are the ones that want wait, and they now have a
way to say so.
The two keys stay separate despite sharing a domain, because a full
quota is "queue for your share" (your turn always comes) while an open
circuit is "wait for the source to recover" (it might not).
Policy validation collapses into SourceAdmission, the only consumer.
The three client constructors used to each carry their own copy of the
quota_full check; adding a second key there would have made eight
copies of the same two lines. Rejection timing and message are
unchanged -- admission is built inside those constructors.
This commit only wires the key through; the control flow that reads it
lands next.
retry_after_s never had a written definition, so each backend improvised
and they drifted apart. It now answers exactly one question: how long
until a retry is *certainly* worth attempting. OPEN has such a moment
(the cooldown deadline); HALF_OPEN does not, because the probe can come
back at any time -- so it reports 0.0, which already means "retry now"
elsewhere in the library.
Six exits are brought in line. The half-open rejection is the one issue
14 reported: it returned the probe lease remainder, a deadlock-guard
value derived from 2x the slowest timeout, so a 60s cooldown told
callers to wait 600s. Worse, retry.py fed that number into the source
cooldown memo, whose set_until only moves forward -- a source stayed
skipped in-process for the whole lease even after its probe succeeded
and the gate closed. That now writes an already-expired deadline, so
the memo goes back to recording only real OPEN cooldowns.
The other five were pre-existing memory/redis divergences hidden by a
contract-test blind spot (the suite pinned that a second caller gets
rejected, never what number it got): redis reported the probe TTL on
grant and the lease remainder on fenced-out writes, where memory has
always reported 0. Contract cases now pin all four half-open exits on
both backends, with 1:1 real-wait variants for redis since the
fake-clock ones skip there.
iomgaa