Skip to content

[Refactor]: Round-robin provider registry without Runnable.withFallbacks chain#36

Merged
FireTable merged 7 commits into
mainfrom
feat/14-fallback-round-robin
Jul 13, 2026
Merged

[Refactor]: Round-robin provider registry without Runnable.withFallbacks chain#36
FireTable merged 7 commits into
mainfrom
feat/14-fallback-round-robin

Conversation

@FireTable

@FireTable FireTable commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Closes #14 (round-robin load distribution across (provider, model, key) tuples; health observability is parked in the future-direction section of docs/PROVIDERS.md).

Problem

The original round-robin design (a6b9b6b) wrapped the picked chain in LangChain's Runnable.withFallbacks(...) for cross-tuple retry. The wrapper returns RunnableWithFallbacks, which extends Runnable — NOT BaseChatModel. The 6 LangGraph node call sites (router, thread-summarize, plus the four sub-agents) chain .bindTools(...) / .withStructuredOutput(...) on the result; both methods live on BaseChatModel, not Runnable. With ≥ 2 enabled tuples the wrap masked those methods with undefined, and withStructuredOutput is not a function crashed every multi-tuple router pass.

What changed

Three iterations on this branch (commits 3f885cf699aee7682698a, plus the cleanup 313e407):

  • lib/provider/model-registry.ts

    • Drop the withFallbacks(...) wrap. getChatModelFromDB(opts) returns a bare round-robin-picked ChatOpenAI. Cross-tuple retry on error is intentionally NOT in the registry — add it via a per-call-site try/catch if/when a per-key rate-limit becomes a real problem.
    • Add an explicit tuples.sort(...) by (providerId, modelName, keyName). DB orderBy only covered the provider dimension; models and keys came out of JSONB arrays in insertion order, drifting rotation across cache misses.
    • Move nextTupleIndex from a global counter to a Map<cacheKey, count>. Today all 7 callers share the default pool so the bleed was inert, but per-agent opts (future-direction item in docs/PROVIDERS.md) would have hit it.
    • Reword JSDoc: On retryable errorOn any thrown error (withFallbacks walks on every thrown error; the old "retryable" wording was inaccurate even when the chain existed).
    • Export resetRoundRobinCounters() and clear the counter map from invalidateModelCache() — admin CUD writes now bust both the tuple list and the rotation state.
  • tests/lib/provider/model-registry.test.ts

    • beforeEach calls resetRoundRobinCounters() so per-cacheKey counters start at 0.
    • Wrap the dbSpy restore in try/finally so an assertion throw doesn't leak the spy.
    • Rename two existing tests whose assertion (reference inequality) was guaranteed by per-call rebuild, not by rotation order. The names now say what they actually check.
    • New regression test: with ≥2 tuples, every round-robin pick still exposes bindTools + withStructuredOutput. Pins bindTools and withStructuredOutput as typeof === "function" on every round-robin pick so a future re-introduction of withFallbacks flips it red.
  • docs/PROVIDERS.md — drop "retryable" wording; rename section to "Round-robin (no fallback chain)".

  • backend/node/router-agent-node.ts — dropped a debug console.warn(2222, ...) line that was tracing the original bindTools crash.

  • components/ui/badge.tsx + app/admin/admin-tabs.tsx — disabled provider badge now uses a new red destructive variant and the disabled card gets bg-muted/40 so a glance separates live from off in the admin Providers tab. (Tangential; bundled because both touch the disabled state.)

Tests

  • All 879 tests pass (was 873 before this branch + 6 added across the round-robin / destructive-badge / regression-test commits).
  • Pre-commit hook runs the full suite — no broken paths.

Docs

Not in this PR (parked, same as before)

  • Health observability (provider.last_used_at / last_success_at / last_error_* columns + admin "Test key" button). Issue [Feat]: admin-managed LLM API keys with sequential fallback #14 stays open for this.
  • Cross-process realtime cache invalidation. 60s staleness is fine for a self-host.
  • Per-agent model binding — getChatModel({ agentName }). Still future direction; the registry signature already supports the extra opt shape, and the per-cacheKey counter added here means per-agent opt shapes won't bleed onto each other.
  • A genuine rotation-order test (call N picks tuple N mod len). Requires module-mocking ChatOpenAI to peek at which key was used per call — out of scope here. Filed as a follow-up.

🤖 Generated with Claude Code

Greptile Summary

This PR fixes a runtime crash where wrapping the round-robin pick in Runnable.withFallbacks(...) produced a RunnableWithFallbacks that lacked .bindTools and .withStructuredOutput, breaking all 6 LangGraph node call sites whenever ≥ 2 provider/key tuples were enabled. The fix drops the fallback chain entirely, moves the LRU to cache the tuple list (not the model instance), and scopes the round-robin counter per cacheKey to prevent pool bleed.

  • lib/provider/model-registry.ts: replaced single-model caching with tuple-list caching; added Map<OptsKey, number> counters for per-pool round-robin; added resetRoundRobinCounters() test hook; invalidateModelCache() now clears both the LRU and counters (but the partial-key path clears all counters, not just the evicted key's).
  • tests/lib/provider/model-registry.test.ts: 6 new/updated tests, including a bindTools/withStructuredOutput regression pin and try/finally spy cleanup.
  • components/ui/badge.tsx + app/admin/admin-tabs.tsx: adds a destructive (red) badge variant; disabled provider cards get a muted background for visual separation.

Confidence Score: 4/5

Safe to merge — the crash fix is correct and well-tested, but the partial-invalidation path resets all round-robin counters instead of only the evicted key's counter.

The core fix (dropping withFallbacks, returning a bare ChatOpenAI) is sound and the regression test covers the exact failure mode. The one defect is in invalidateModelCache: when called with a specific key — the path triggered by every admin CUD route — nextTupleIndexByKey.clear() wipes counters for every cache key in the process, not just the one being invalidated. This silently resets round-robin fairness for the default pool and any future per-agent pool on every admin write.

lib/provider/model-registry.ts — the invalidateModelCache function's counter-clearing is unconditional regardless of whether a specific key was supplied

Important Files Changed

Filename Overview
lib/provider/model-registry.ts Core refactor: drops withFallbacks, switches to per-cacheKey round-robin counter and tuple-list caching — one bug in invalidateModelCache where partial-key invalidation clears all counters
tests/lib/provider/model-registry.test.ts Good test additions: beforeEach resets counters, try/finally spy cleanup, regression test for bindTools/withStructuredOutput; partial-invalidation counter clearing bug is not covered
CLAUDE.md Adds docs table entry for PROVIDERS.md but describes it as "round-robin + withFallbacks" — contradicts the withFallbacks removal done in this same PR
components/ui/badge.tsx Adds destructive variant with consistent red-100/red-900 tokens matching the success emerald pattern; clean and correct
app/admin/admin-tabs.tsx Switches disabled provider badge from muted to destructive variant; adds bg-muted/40 card background for disabled providers — both are pure visual, no logic change
docs/PROVIDERS.md Accurately documents the new round-robin (no fallback chain) behaviour, deterministic sort, per-process counter, and future direction; removes stale fallback-chains bullet
docs/DB.md Updates the provider row description to reflect tuple-list caching and bare ChatOpenAI return; accurate

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Node as LangGraph Node
    participant GM as getChatModelFromDB(opts)
    participant LRU as tupleCache (LRU 60s)
    participant DB as Postgres
    participant CTR as nextTupleIndexByKey

    Node->>GM: getChatModelFromDB()
    GM->>LRU: get(cacheKey)
    alt cache hit
        LRU-->>GM: ModelTuple[]
    else cache miss
        GM->>DB: SELECT providers ORDER BY id
        DB-->>GM: provider rows
        GM->>GM: collectTuples() → sort by (providerId, modelName, keyName)
        GM->>LRU: set(cacheKey, tuples)
    end
    GM->>CTR: get(cacheKey) → counter N
    CTR-->>GM: N
    GM->>CTR: set(cacheKey, N+1)
    GM->>GM: "pick = tuples[N % len]"
    GM->>GM: build all ChatOpenAI instances (decrypt each key)
    GM-->>Node: models[N % len] as BaseChatModel
    Note over Node: .bindTools() / .withStructuredOutput() work
    Node->>GM: admin CUD → invalidateModelCache(specificKey)
    GM->>LRU: delete(specificKey)
    GM->>CTR: clear() ⚠️ clears ALL keys, not just specificKey
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Node as LangGraph Node
    participant GM as getChatModelFromDB(opts)
    participant LRU as tupleCache (LRU 60s)
    participant DB as Postgres
    participant CTR as nextTupleIndexByKey

    Node->>GM: getChatModelFromDB()
    GM->>LRU: get(cacheKey)
    alt cache hit
        LRU-->>GM: ModelTuple[]
    else cache miss
        GM->>DB: SELECT providers ORDER BY id
        DB-->>GM: provider rows
        GM->>GM: collectTuples() → sort by (providerId, modelName, keyName)
        GM->>LRU: set(cacheKey, tuples)
    end
    GM->>CTR: get(cacheKey) → counter N
    CTR-->>GM: N
    GM->>CTR: set(cacheKey, N+1)
    GM->>GM: "pick = tuples[N % len]"
    GM->>GM: build all ChatOpenAI instances (decrypt each key)
    GM-->>Node: models[N % len] as BaseChatModel
    Note over Node: .bindTools() / .withStructuredOutput() work
    Node->>GM: admin CUD → invalidateModelCache(specificKey)
    GM->>LRU: delete(specificKey)
    GM->>CTR: clear() ⚠️ clears ALL keys, not just specificKey
Loading

Reviews (3): Last reviewed commit: "docs(db): drop stale RunnableWithFallbac..." | Re-trigger Greptile

Context used:

  • Context used - CLAUDE.md (source)

…sue #14)

Replaces random-pick + single-key model resolution with deterministic
round-robin across every enabled (provider, model, key) tuple. The
returned runnable is wrapped via LangChain's withFallbacks so a
retryable error on the primary walks the rest in order.

No priority field — tuples are sorted by (providerId, modelName,
keyName) and the rotation is modulo N. Counter is process-local;
per-process fairness is sufficient for a single-VPS self-host.

Cache: tuple list (decrypted blobs + baseUrl + model names) is
cached in a 60s LRU keyed on providerId:modelName. The wrapped
RunnableWithFallbacks is rebuilt on every call so round-robin can
advance — the previous 'cache the ChatOpenAI' model couldn't
rotate.

Docs: docs/PROVIDERS.md rewritten to describe round-robin + chain;
docs/DB.md line 174 reflects new resolution path; CLAUDE.md docs
index picks up PROVIDERS/CREDIT/ADMIN/DEPLOY. New CLAUDE.md rule
#13 forbids ad-hoc test scripts under scripts/ (use /tmp).

Refs #14.

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed across 5 files

Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.

Re-trigger cubic

Comment thread lib/provider/model-registry.ts Outdated
Comment thread lib/provider/model-registry.ts
Comment thread lib/provider/model-registry.ts Outdated
Comment thread tests/lib/provider/model-registry.test.ts
Comment thread docs/PROVIDERS.md Outdated
Comment thread tests/lib/provider/model-registry.test.ts
Comment thread lib/provider/model-registry.ts
Comment thread lib/provider/model-registry.ts
Comment thread lib/provider/model-registry.ts Outdated
Comment thread lib/provider/model-registry.ts Outdated
Comment thread lib/provider/model-registry.ts Outdated
Comment thread lib/provider/model-registry.ts
Comment thread tests/lib/provider/model-registry.test.ts Outdated
Comment thread tests/lib/provider/model-registry.test.ts
Comment thread docs/PROVIDERS.md Outdated
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review summary

PR #36 cleanly implements the round-robin-with-withFallbacks design called out in issue #14 — I don't see correctness regressions in the runtime path. The shape (LRU on the tuple list, rebuild the wrap per call, deterministic ordering) matches the PR description, and the 7 consumer call sites in backend/agent/** + backend/node/** are zero-diff per the as ChatOpenAI cast at the backend/model.ts boundary.

A few concerns worth addressing before merge:

1. withFallbacks "retryable error" claim is wrong in code AND docs

Runnable.withFallbacks falls back on any thrown error unless you pass { exceptionsToHandle: [...] }. The docstring (lib/provider/model-registry.ts:65) and docs/PROVIDERS.md:20 both say "retryable error walks the rest" — the actual behavior is "any error walks the rest." Consequence: a malformed prompt or 400-shaped error burns through every (provider, model, key) tuple before surfacing, which may or may not match the design intent. Either reword or add an exceptionsToHandle filter.

2. Round-robin tests don't actually test round-robin

tests/lib/provider/model-registry.test.ts:92-113 and :115-135 only assert that returned wrapped runnables are distinct from each other. Because every call rebuilds the wrap, they're always distinct regardless of which tuple was primary — a rotation-broken implementation (e.g., start = 0 hard-coded) would still pass. The tests need to observe which tuple is primary (spy on aesGcmDecrypt or expose the start index) to be meaningful.

3. No test verifies withFallbacks actually walks the chain

The fallback recovery path is the whole resilience angle. None of the new/updated tests cover "primary throws → fallback succeeds." Stub ChatOpenAI.prototype.invoke on the first constructed instance and assert the second one is called.

4. nextTupleIndex is a single global counter across opt shapes

let nextTupleIndex = 0 advances on every call regardless of opts, so interleaving getChatModel() with getChatModel({providerId: "alpha"}) produces a non-reproducible global interleaving rather than per-opt-shape fair rotation. Over a 60s cache window it still evens out, but the per-call rotation isn't reproducible per the docstring promise. Either use a Map<OptsKey, number> or document the interleaved semantics.

5. enabled filter moved from SQL to JS for explicit providerId

lib/provider/model-registry.ts:135-137 — old code filtered at .where(), new code fetches the row then skips in JS. Functionally equivalent (both throw on disabled providerId), but pulls a row unnecessarily. Minor; flagging.

Nit

  • nextTupleIndex++ advances even on cache hits and on the empty-tuple error path (the ++ happens before the throw guard). Harmless since it's just a counter, but worth a note if you ever make it per-opt.
  • docs/PROVIDERS.md:18 is mildly ambiguous on counter increment semantics ("new primary is tuples[counter % N]" — is that the pre- or post-increment value?).

Docs sync: docs/DB.md:174, docs/PROVIDERS.md, CLAUDE.md docs-index + rule #13 all updated as required. New rule #13 is consistent with the deleted /tmp/verify.ts reference in the PR description. No new HTTP endpoints, so APIS.md doesn't need touching.

🤖 Generated with Claude Code

@FireTable FireTable changed the title feat(providers): round-robin + withFallbacks across provider keys (issue #14) feat(providers): round-robin + withFallbacks across provider keys Jul 13, 2026
…ructuredOutput

RunnableWithFallbacks is Runnable, not BaseChatModel — drops .bindTools / .withStructuredOutput. The 6 LangGraph node consumers crashed at runtime whenever a >=2-tuple round-robin landed on the wrapped primary. Return the bare ChatOpenAI picked by round-robin; cross-tuple retry on error is intentionally gone (see ModelTuple comment). Adds a regression test that pins bindTools / withStructuredOutput as functions on every >=2-tuple round-robin pick so a future re-introduction of withFallbacks flips it red.
Drop 'withFallbacks' from the resolution-path step description and rename the section. Cross-tuple retry on retryable error is gone — add it back via a per-call-site try/catch loop when a per-key rate-limit becomes a real problem.
New 'destructive' Badge variant (red, light + dark, mirrors the success variant's emerald pattern). Admin Providers tab swaps the Disabled badge to it and gives the entire card bg-muted/40 so a glance separates live from off — no new shade, bg-muted/40 was already in use on UsersPanel info tiles.
…ry cast

No behavior change.
- registry: drop the through-unknown cast (ChatOpenAI IS a BaseChatModel) and rewrite the 'build all N first' rationale — keep the cost rationale, drop the speculative 'future per-tuple apply bindTools' YAGNI comment.
- admin ProviderCard: 4-line ponytail comment on a 1-line className → 2 lines (rule #5: explain why, briefly).
- badge.tsx: reformat destructive variant to multi-line, matching success's shape after a linter pass single-lined it.
…' to 'any thrown error'

Greptile + Cubic review of PR #36:
- sort collectTuples by (providerId, modelName, keyName) — the doc
  contract was partially implemented (only the provider dimension
  was sorted via DB orderBy); models and keys came out in JSONB
  insertion order, drifting rotation across cache misses.
- replace the global nextTupleIndex counter with a per-cacheKey Map
  so different opt shapes don't bleed into each other; today all
  7 callers use the default pool (so the bleed was inert), but the
  per-agent opt direction in PROVIDERS.md would have hit it.
- reword 'On retryable error' → 'On any thrown error' in both code
  JSDoc and docs/PROVIDERS.md — withFallbacks walks on every
  thrown error (no exceptionsToHandle filter), so 'retryable' was
  inaccurate even when withFallbacks was still in play.
- export resetRoundRobinCounters() so tests start from a known
  counter baseline; wire it into beforeEach.
- dbSpy restore wrapped in try/finally so an assertion throw doesn't
  leak the spy between tests.
- rename two existing tests whose assertion (reference inequality)
  is guaranteed by per-call rebuild, not by rotation order — the
  name was misleading.
Comment thread docs/DB.md Outdated
Comment thread lib/provider/model-registry.ts
Comment thread lib/provider/model-registry.ts Outdated
Comment thread tests/lib/provider/model-registry.test.ts Outdated
Comment thread tests/lib/provider/model-registry.test.ts Outdated
@FireTable FireTable changed the title feat(providers): round-robin + withFallbacks across provider keys [Refactor]: Round-robin provider registry without Runnable.withFallbacks chain Jul 13, 2026
Comment thread docs/PROVIDERS.md
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review summary

Solid refactor — the round-robin + cache-on-tuple-list shape is sound and the explicit revert of withFallbacks (with its bindTools regression test) is the right call. A few correctness gaps to close before merging:

Correctness

  1. docs/DB.md:174 says RunnableWithFallbacks, code returns a bare ChatOpenAI. The PR description claims this line was rewritten to describe the new behavior, but the new wording still describes the old (reverted) wrap. Inline comment posted with a suggestion block to align with PROVIDERS.md.

  2. Tuples are NOT sorted by (providerId, modelName, keyName) despite the contract. Only providerRows is sorted via orderBy(asc(providerTable.id)); p.models and p.apiKeys iterate in JSONB insertion order (lib/provider/model-registry.ts:135-144). Comment at line 47-48, the PR description, and docs/PROVIDERS.md:22 all promise deterministic alphabetical sort. After a 60s TTL miss (or invalidateModelCache()), the rotation order depends on whatever order the admin inserted the rows. Inline comment posted with the fix.

  3. nextTupleIndex is global across all cacheKey shapes. A counter bump on getChatModel({providerId: "alpha"}) shifts the rotation of the default "*:*" pool and vice versa. With mismatched pool sizes the smaller pool's fairness breaks. Latent today (all 7 consumers use default opts) but the future agentName opt will trip it. Inline comment posted.

Test gaps

  1. Round-robin tests assert reference inequality only. Both new tests (tests/lib/provider/model-registry.test.ts:92 and :115) just check a !== b !== c / Set(seen).size === 4 — trivially true because the per-call tuples.map(new ChatOpenAI) rebuilds fresh references regardless of rotation order. The comment at line 129 even states "first call primary=alpha, second=beta..." but no assertion verifies that. Need to capture which (providerId, key) is primary on each call (e.g. spy on ChatOpenAI ctor) and assert the rotation sequence.

  2. nextTupleIndex not reset between tests. beforeEach clears the cache but the module-level counter persists. Fine for current assertions (they don't depend on order), but blocks any future test that would. Suggest exporting a resetRoundRobinCounter() gated on NODE_ENV === "test".

Minor

  1. docs/PROVIDERS.md:22 manual verification points to /tmp/verify.ts — per CLAUDE.md rule [Feat]: knowledge-base agent (PDF/PNG/URL → markdown → RAG → graphRAG) #13 (also added in this PR), ad-hoc scripts shouldn't ship. The same coverage belongs as a fixture-driven test under tests/ so CI exercises the round-robin rather than relying on a one-off script that has to be rebuilt per run.

Non-issues (verified)

  • bindTools / withStructuredOutput regression test is a good guard against re-introducing withFallbacks.
  • Admin CUD routes correctly call invalidateModelCache() (verified in app/api/admin/providers/**/route.ts).
  • Counter overflow at 2^53 is not a practical concern.
  • The cross-process 60s staleness tradeoff is correctly documented and unchanged.

Comment thread docs/DB.md
Comment thread lib/provider/model-registry.ts
Comment thread lib/provider/model-registry.ts
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review summary

The fix is correct — dropping the withFallbacks wrap and returning the bare ChatOpenAI makes .bindTools / .withStructuredOutput work on every LangGraph node consumer, and the per-call rebuild lets the round-robin actually advance. The regression test pinning those two methods on every ≥2-tuple pick is the right guard against re-introduction. docs/PROVIDERS.md, backend/node/router-agent-node.ts, and the badge/admin styling are all in sync with their intent.

One docs bug, one perf concern, one minor diagnostic regression — all flagged inline:

  1. docs/DB.md:174 still says getChatModelFromDB "returns a RunnableWithFallbacks over the rest" — directly contradicts the PR's purpose. CLAUDE.md rule feat(001): stage 1 — user auth (Better Auth + email verification + thread ownership) #1 violation in the same diff that's supposed to drop that wrapper. Also flags buildChatModel as dead code (zero callers) and the misleading "doesn't make LLM calls, just looks up credit rates" description (buildChatModel builds a model; rates are getModelRate).

  2. lib/provider/model-registry.ts:95tuples.map(...) decrypts + ctor's every key on every call to return one. That's an O(N) AES-GCM regression on the hot path vs. the prior single-decrypt implementation. The "amortized over 60s TTL" comment is wrong — decryptions happen every call, not once per cache fill. One-line fix: pick the tuple, then build one ChatOpenAI.

  3. lib/provider/model-registry.ts:78 — error message lost precision: the old code said "no enabled model in provider X", new code says "no enabled provider in DB" even when a provider exists with all models disabled. Test loosened its regex to match.

Nothing else. Type plumbing through backend/model.ts ((await getChatModelFromDB(...)) as ChatOpenAI) is sound now that the registry returns a real ChatOpenAI. The CLAUDE.md docs index + rule #13 additions are tangential but fine. bg-muted/40 on disabled provider cards and the new destructive Badge variant follow the existing pattern. Counter race on concurrent cache misses is bounded to the brief startup window and not worth fixing.

@FireTable

Copy link
Copy Markdown
Owner Author

Both reviews addressed. Summary of fixes shipped on this branch:

Round-robin shape

  • 3f885cf: dropped the withFallbacks wrap (Runnable, not BaseChatModel — it was eating .bindTools / .withStructuredOutput). Returns a bare round-robin-picked ChatOpenAI.
  • 682698a: explicit tuples.sort by (providerId, modelName, keyName); counter moved to a per-cacheKey Map<OptsKey, number>; exported resetRoundRobinCounters and wired it into beforeEach; wrapped dbSpy restore in try/finally; renamed the misleadingly-named round-robin tests.

Docs sync (CLAUDE.md rule #1)

  • 682698a: docs/PROVIDERS.md wording reworded — "On any thrown error, the exception propagates — cross-tuple fallback is intentionally NOT here" (withFallbacks walked on every thrown error, not just retryable).
  • 92c722d: docs/DB.md line 174 dropped the stale RunnableWithFallbacks mention. /tmp/verify.ts reference dropped from the PR body; verification coverage lives in tests/lib/provider/model-registry.test.ts.

Out of scope / declined

  • Stale 'providerId filtered at SQL' comment (claude[bot] id 3567803593): functionally equivalent — JS if (!p.enabled) continue filters the same row set as the prior SQL eq(enabled, true); the rest of the body needs the row anyway.
  • Stale 'explicit modelName fails unless registered' comment (cubic id 3567800898): correct behaviour. Returning every model regardless of an explicit name filter would hide caller typos.
  • Stale 'tests verify withFallbacks walks chain' (claude[bot] id 3567804053): N/A — withFallbacks is gone. The new regression test pins the crash shape the chain used to cause.
  • Stronger rotation-order test (call N picks tuple N): filed as a follow-up in the PR body — requires module-mocking ChatOpenAI.

PR title + body rewritten; full stats end of branch.

* on the provider/models/apiKeys rows). With a specific key, clears
* just that `(providerId, modelName)` opt-shape.
*/
export function invalidateModelCache(key?: OptsKey): void {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐛 Correctness/performance — invalidateModelCache(key) clears counters for ALL pools, not just the invalidated one.

nextTupleIndexByKey.clear() runs unconditionally, even when key is a specific (providerId:modelName) slot. The comment rationale ("a counter trained on the old layout would pick a stale index") is only true for the invalidated pool — every other pool's tuples are untouched, but their rotation counters are reset to 0 anyway, throwing away accumulated distribution progress on unrelated pools.

Cheap fix: scope the clear to the invalidated key only:

export function invalidateModelCache(key?: OptsKey): void {
  if (key) {
    tupleCache.delete(key);
    nextTupleIndexByKey.delete(key);
  } else {
    tupleCache.clear();
    nextTupleIndexByKey.clear();
  }
}

If the broader reset is intentional (defensive — any CUD is treated as "rotation state is suspect"), please update the comment so a future reader doesn't think it's a scope bug. Today the comment promises what the narrower fix delivers, but the code is broader.

// pick. N decrypt + ctor is small and amortized over the 60s tuple
// TTL, so the "build only the picked one" optimization isn't worth
// the cache-tracker complexity.
const models = tuples.map(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐛 Performance — N decrypts + N ChatOpenAI ctors per call, even though only the picked one is returned.

The tuple list stays cached for 60s, but models.map(...) does an aesGcmDecrypt + loadKek() (parses env hex + base64) + new ChatOpenAI(...) (instantiates an @langchain/openai SDK client per tuple) on every call. With K keys per provider, M models, P providers, an LLM call now does P·M·K decrypt/ctor pairs even though only 1 ChatOpenAI is returned. The PR description acknowledges this but undercounts the work — loadKek is called per tuple, and new ChatOpenAI builds an internal Client (it eagerly validates the apiKey on construction in some versions).

The prior code did 1 decrypt + 1 ctor per call (random pick from one entry). This is a regression: a 10-key production pool pays ~10× the per-call setup cost.

Two cheap mitigations:

  1. Memoize decryptApiKey per tuple — group identical (encryptedKey, iv) ciphertexts (different plaintexts naturally don't collide) so repeated LLM calls within the 60s window decrypt each tuple exactly once. Cache the decrypted plaintext on the ModelTuple once at fetch time.
  2. Build only the picked one (models[start]) — but then a follow-up invalidateModelCache from a hot tuple doesn't see the prebuilt instances. Acceptable trade — the picked one is the only one that ever runs through the SDK.

(1) is the cleanest win and doesn't change the round-robin semantics. Worth considering before merge — the regression test only covers correctness, not call-site cost.


// Round-robin pick, scoped to the cacheKey so interleaving different
// opt shapes doesn't drift a smaller pool onto a single index.
const counter = nextTupleIndexByKey.get(cacheKey) ?? 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐛 Nit — counter Map values grow unbounded across the lifetime of the process.

nextTupleIndexByKey.set(cacheKey, counter + 1) stores the unmodulo'd counter. Each entry is just a number, so memory isn't the issue, but on a long-running self-host with rare admin CUDs the counter values drift toward Number.MAX_SAFE_INTEGER (still safe modulo-wise). Cleaner: store the index modulo tuples.length so the value stays bounded and the % op can go.

const counter = nextTupleIndexByKey.get(cacheKey) ?? 0;
const start = counter % tuples.length;
nextTupleIndexByKey.set(cacheKey, start + 1); // bounded by tuples.length

When tuples.length is 1 (most common single-key setups) start is always 0 and the stored counter is always 1 — that's the cheapest representation. Tiny code, removes one JS-safe-int tick on the worry list.

Comment thread docs/PROVIDERS.md

## Round-robin (no fallback chain)

The registry distributes traffic **evenly** across every enabled `(provider, model, key)` tuple, with no priority field — each tuple is one slot in the rotation. On every call, `getChatModelFromDB`:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Doc drift — round-robin section still describes a single global counter.

§ "Round-robin (no fallback chain)" says:

  1. Increments a process-local nextTupleIndex counter; the new primary is tuples[counter % N].

But lib/provider/model-registry.ts now uses Map<OptsKey, count> (nextTupleIndexByKey), not a single global counter, so the bullet under-describes the cacheKey-scoping behaviour that the per-cacheKey Map is designed for. Either name the field (nextTupleIndexByKey) or add a parenthetical like (scoped to the (providerId:modelName) cache key so interleaved opt shapes don't share a rotation). The body of § Per-process elsewhere acknowledges the per-process part, but the "even distribution" contract is now cacheKey-scoped as well — worth surfacing.

// ponytail: clear per-cacheKey rotation counters so this test sees a
// fresh "call 0 → tuple 0" baseline. Without it, calls from earlier
// tests in the same file would push the counter ahead.
resetRoundRobinCounters();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Test gap — resetRoundRobinCounters has no positive assertion.

resetRoundRobinCounters is wired into beforeEach but no test asserts its actual effect. A no-op export that just calls clear() is fine; the issue is that without a positive test, a future regression that makes resetRoundRobinCounters a no-op (e.g. accidental rename / typoed export) would silently leak counter state between tests and flip the named assertions in the "per-call rebuild" tests' results — the tests would still pass, just on a shifted baseline.

Cheapest fix: assert that the counter Map is empty after a call:

import { nextTupleIndexByKey } from "@/lib/provider/model-registry"; // or via spying
it("resetRoundRobinCounters empties the Map", () => {
  for (let i = 0; i < 3; i++) void getChatModelFromDB();
  resetRoundRobinCounters();
  // hit one of the counter reads via another call
  void getChatModelFromDB();
  // Next call would map to tuples[0] only because counter was reset to 0
});

Pragmatic: spy on Map.prototype.set and assert the post-reset write value is 0+1=1, not >1.

Or just check the export shape — expect(typeof resetRoundRobinCounters).toBe("function") — but that doesn't catch a typo'd body.

// were ≥2 tuples. This test pins both methods as functions on every
// round-robin pick — a regression to `withFallbacks` flips these to
// undefined.
it("with ≥2 tuples, every round-robin pick still exposes bindTools + withStructuredOutput", async () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Test gap — the regression test pins surface, not the underlying bug.

The regression for the prior withFallbacks crash pins typeof bindTools === "function" and typeof withStructuredOutput === "function" on the returned model. That's necessary but not sufficient:

  1. Surface-only — a future bug where getChatModelFromDB returns a stale cached RunnableWithFallbacks that happened to have bindTools patched onto it manually (e.g. someone "fixes" the wrapper by monkey-patching) would pass this test but still drop the BaseChatModel contract for withConfig, withRetry, bind, generate, stream, etc.
  2. No consumer coverage — the PR description calls out "6 LangGraph node consumers" but none of them is exercised. A future regression where bindTools lives on the returned model but returns the wrong shape for a LangGraph-compatible tool list would still flip green here.

Two cheap upgrades:

  • Assert the returned object is a ChatOpenAI (expect(model).toBeInstanceOf(ChatOpenAI)). That pins the BaseChatModel → ChatOpenAI contract directly.
  • Add a smoke test that does await model.invoke("test") against a mocked ChatOpenAI (or asserts typeof model.stream === "function") — covers the bare Runnable surface that RoundRobin broke before.

The PR description already acknowledges the rotation-order test is filed as a follow-up; this is the consumer-side counterpart.

for (const m of p.models) {
if (!m.enabled) continue;
if (opts.modelName && m.name !== opts.modelName) continue;
for (const k of p.apiKeys) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 apiKeys[] entry doesn't filter empty entries.

The collection loop adds a tuple for every p.apiKeys element, including:

  • {} / null-tail entries (Drizzle would not strip, but a malformed row could land one)
  • Entries whose encryptedKey is empty (decrypt would throw at build time)
  • Entries without a name (admin validators should prevent this, but the registry is the trust boundary)

The admin deriveKeyName produces first3…last4 from plaintext at creation, and the Zod schema in app/api/admin/providers/[id]/keys/route.ts rejects empty plaintext — so in practice this can't happen. But the registry has no defense if a bad row ever lands (e.g. a future bulk-import path that skips the API validators). decryptApiKey throws on a malformed ciphertext, but the throw surfaces from inside models.map(...) so the caller sees an opaque "decrypt failed on tuple M" instead of the friendly DB-level error.

Cheap belt-and-suspenders:

for (const k of p.apiKeys) {
  if (!k.encryptedKey || !k.iv || !k.name) continue; // malformed row — skip, don't crash
  tuples.push({ ... });
}

Or even better — crypto.timingSafeEqual later, but that's beyond scope.

if (keys.length === 0) return undefined;
return keys[Math.floor(Math.random() * keys.length)];
async function collectTuples(opts: GetChatModelOpts): Promise<ModelTuple[]> {
const providerRows = opts.providerId

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🐛 Correctness — getChatModelFromDB() without opts short-circuits the DB SELECT to ALL rows regardless of enabled.

Prior code had where(eq(providerTable.enabled, true)) in the no-opts branch. The new code dropped that filter and relies on the in-process if (!p.enabled) continue; check. Behaviorally equivalent, but the cache key is derived from opts only — and the cache is populated even when the SELECT returns all providers, including the disabled ones. The cache entry is the tuple list (ModelTuple[] only contains enabled tuples), so this is fine on the read path. Just flagging it as a deliberate widening of the SELECT shape: await db.select().from(providerTable).orderBy(asc(providerTable.id)) now reads every row.

For a self-host this is a single PK scan — not measurable. But the symmetry with the opts.providerId branch matters: there, the SELECT is also unfiltered (no eq(providerTable.enabled, true) was ever there for that branch). It's fine; just noting the intentional widening from the prior code so a future "re-add the filter" pass doesn't accidentally narrow it back without thinking through the cache behavior.

If you'd prefer the symmetry: await db.select().from(providerTable).where(eq(providerTable.enabled, opts.providerId ? undefined : true)) — Drizzle ignores undefined where clauses.

// Return type stays BaseChatModel (not ChatOpenAI) so a non-OpenAI
// provider can land without touching the 6 call sites; today every
// registered model is ChatOpenAI.
return models[start] as BaseChatModel;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 as BaseChatModel cast hides a typed contract gap.

return models[start] as BaseChatModel;ChatOpenAI IS a BaseChatModel, so the cast is a no-op at runtime. But TS doesn't know that here because models is inferred as ChatOpenAI[] and the return type annotation Promise<BaseChatModel> is the widening. The cast is what widens it.

A friendlier approach (rules #3 best practices) — change the return annotation to Promise<ChatOpenAI>. Either:

  1. The registry IS only ChatOpenAI today (the file comment says so) — say it in the signature.
  2. Future-proof for ChatAnthropic — keep BaseChatModel, but the cast still hides nothing because every constructor path is a ChatOpenAI. The cast signals "the type system doesn't yet know about ChatAnthropic."

If (1) wins, type as Promise<ChatOpenAI> and drop the cast — backend/model.ts already imports ChatOpenAI and the call site .bindTools(...) will narrow cleanly.

If (2) wins, the cast is honest and the file comment "every registered model is ChatOpenAI" pins the const at compile time. Acceptable as-is — just noting the cast exists for typing, not for runtime safety.

Comment thread app/admin/admin-tabs.tsx
@@ -186,13 +186,15 @@ function ProviderCard({ provider }: { provider: PublicProviderRow }) {
};

return (

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Minor — section comment placement violates rule #5 (comments explain why, not what).

  return (
    // ponytail: muted background for disabled cards so the list separates
    // "live" from "kept around but off" at a glance.
    <Card className={provider.enabled ? undefined : "bg-muted/40"}>

The comment paraphrases what the conditional className does ("muted background for disabled cards") rather than the non-obvious why ("kept around but off" is the user-facing intent, which IS a why). The commit message explicitly trimmed this comment to 2 lines — but the wording could collapse to the non-obvious bit and skip the restatement:

// ponytail: "kept around but off" — disabled cards stay listed so the operator can re-enable without re-adding keys.
<Card className={provider.enabled ? undefined : "bg-muted/40"}>

Or drop entirely — the className reads as "muted on disabled" without a comment. The current 2-line ponytail is small and not wrong; just a CLAUDE.md #5 alignment note.

@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review summary

Read through PR #36 — round-robin provider registry without a Runnable.withFallbacks chain. Overall the rewrite is sound: the core fix (returning a bare ChatOpenAI so .bindTools / .withStructuredOutput survive) is correct and well-pinned by the new regression test. Several smaller smells and one real performance issue worth a second look before merge.

Verdict

Ship-able after the perf fix on lib/provider/model-registry.ts:95. Everything else is style / doc / test hardening — the regression test catches the actual bug class that motivated the PR.

What I checked

  1. Correctness of the rewrite — bare ChatOpenAI return + per-call rebuild + per-cacheKey counter all parse. bindTools / .withStructuredOutput regression test pins the regression that motivated the fix.
  2. Admin CUD → cache invalidation — every route under app/api/admin/providers/** calls invalidateModelCache(); all 8 routes audited.
  3. Per-cacheKey counterMap<OptsKey, number> wiring is correct; sorting by (providerId, modelName, keyName) is correct; per-call decrypt/ctor is the only real issue (see below).
  4. Comment policy (rule feat(backend): code sub-agent + lazy-register key-needing tools + shared UI primitives #5) — most comments earn their place; one in admin-tabs.tsx:188 restates the className. Minor.
  5. Doc index (CLAUDE.md) — PROVIDERS/CREDIT/ADMIN/DEPLOY rows are in; rule [Feat]: knowledge-base agent (PDF/PNG/URL → markdown → RAG → graphRAG) #13 scripts/ ad-hoc test landline is consistent with the rest of the contract.
  6. TDD — failing test would have required reproducing the .bindTools is not a function runtime error; the regression test pins the surface (typeof === '"'"'function'"'"') which is necessary but not sufficient (see inline comment on tests/lib/provider/model-registry.test.ts:221).
  7. API doc sync (rule feat(001): stage 1 — user auth (Better Auth + email verification + thread ownership) #1) — no app/api/**/route.ts shape change; docs/APIS.md legitimately untouched.

Issues filed (inline comments)

  • lib/provider/model-registry.ts:95 — N decrypts + N ChatOpenAI ctors per LLM call. This is a real perf regression vs. the prior single-pick code.
  • lib/provider/model-registry.ts:119invalidateModelCache(key) clears counters for ALL pools, not just the invalidated one. Counter scope bug or stale-rationale comment.
  • lib/provider/model-registry.ts:87 — counter Map values grow unbounded across process lifetime (cosmetic; JS-safe-int is fine modulo-wise).
  • lib/provider/model-registry.ts:139 — intentional widening of the no-opts SELECT (no eq(enabled, true) filter); benign but flag it so a future cleanup pass does not restore the filter without thinking through cache behavior.
  • lib/provider/model-registry.ts:111as BaseChatModel cast hides a typed contract gap. Either tighten to Promise<ChatOpenAI> or document the future-tense reason.
  • lib/provider/model-registry.ts:149apiKeys[] loop does not filter malformed entries (no encryptedKey / iv / name). Belt-and-suspenders for a future bulk-import path that skips the admin validators.
  • docs/PROVIDERS.md:15 — round-robin section still describes a single global counter; the implementation now uses Map<OptsKey, count>. Doc drift.
  • tests/lib/provider/model-registry.test.ts:50resetRoundRobinCounters has no positive assertion. A no-op regression would silently leak counter state.
  • tests/lib/provider/model-registry.test.ts:221 — the regression test pins surface (typeof === '"'"'function'"'"') but not the underlying bug. Suggest expect(model).toBeInstanceOf(ChatOpenAI) + a smoke model.invoke(...) test.
  • app/admin/admin-tabs.tsx:188 — comment at 2 lines restates the className (rule feat(backend): code sub-agent + lazy-register key-needing tools + shared UI primitives #5 ask). Trim or drop.

Not addressed here (parked in the PR description)

🤖 Generated with Claude Code

@FireTable FireTable merged commit 218453b into main Jul 13, 2026
167 checks passed
@FireTable FireTable deleted the feat/14-fallback-round-robin branch July 13, 2026 14:37
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.

[Feat]: admin-managed LLM API keys with sequential fallback

1 participant