Skip to content

fix(api): restore the background-refresh injection point on /v1/models (#11551) - #11581

Closed
ntdat812 wants to merge 2 commits into
cheaper-inference:release/v3.8.51from
ntdat812:fix/models-after-scheduler-11551
Closed

ntdat812 wants to merge 2 commits into
cheaper-inference:release/v3.8.51from
ntdat812:fix/models-after-scheduler-11551

Conversation

@ntdat812

@ntdat812 ntdat812 commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

src/app/api/v1/models/route.ts passes a third argument to getUnifiedModelsResponse:

return getUnifiedModelsResponse(request, {}, { scheduleBackgroundRefresh: (task) => after(task) });

getUnifiedModelsResponse has accepted two parameters since #9199 removed the injection
point, so that object has been silently discarded. catalogCache.ts never imported after at
all and kept scheduling the stale-while-revalidate rebuild with setTimeout(…, 0).

setTimeout(…, 0) only yields one turn of the event loop — the rebuild still runs while the
response is in flight. A catalog build that blocks for hundreds of milliseconds therefore delays
the stale body the client was promised, which is exactly the flush-before-rebuild guarantee
#8728 exists to provide. It was not in effect.

  • catalogCache gains an exported BackgroundRefreshScheduler type and a
    defaultBackgroundRefreshScheduler built on after(). It falls back to a macrotask when
    after() throws — that happens outside a Next request scope (unit tests, scripts importing
    the module directly), where there is no response to flush and the ordering is meaningless.
  • resolveCachedCatalogResponse accepts scheduleBackgroundRefresh and
    getStaleWhileRevalidateMs on its existing options object. Neither participates in the cache
    key — buildCatalogCacheKey still reads only hideAutoCombos / hideNoThinkVariants.
  • getUnifiedModelsResponse accepts the third argument the route has been passing all along and
    forwards it.

Related Issues

The issue's diagnosis was already exact — route, call site, line numbers and both prior PRs. I
verified each claim against release/v3.8.51 rather than re-deriving it, and answered the one
question it left open (below).

Validation

  • Change type: routing / API
  • Focused tests and category gates from the golden path — see below
  • npm run lint — clean on all three files (eslint exit 0)
  • Reconciled with the current active release base — rebased onto origin/release/v3.8.51
    at 6eb7bf0ba9 (2026-08-26); focused checks rerun after the rebase, same results
  • Production-code changes include a new or updated automated test in this PR — the two
    tests in tests/integration/v1-models-swr-response-flush-8728.test.ts were deliberately
    left red
    by the issue as the spec; this PR turns them green rather than adding new ones.
    They are unmodified.

the /v1/models route wires Next after() as its response-flush-safe scheduler — not ok on
base, ok on this branch.

an external client receives the stale body before synchronous refresh finishes blocking —
I could not run this one here, and I would rather say so than imply I did. It listens on an
AF_UNIX socket at a filesystem path, which Windows refuses with EACCES; the test skips only on
EPERM, so it errors instead of skipping. That is environmental — it fails identically on an
unmodified tree.

So I verified its assertions with an identical harness over TCP — same builder, same
BLOCK_MS = 300 synchronous rebuild, same injected scheduler deferring to outgoing.once("finish"),
same five assertions:

base this branch
scheduledCount (must be 1) 0 — injected scheduler never called 1
stale body reaches client — (assertion never reached) "old"
refresh starts after response finish — +0 ms after flush
client received stale body before refresh ended — 294 ms before
refresh exercised the blocking window — 301 ms

scheduledCount === 0 on base is the whole bug in one number: the scheduler the route passes is
never invoked.

Regression sweep — tests/unit/*catalog*.test.ts + *models*.test.ts, 1180 tests:
1174 pass / 5 fail, identical on base and on this branch. The 5 are pre-existing and
unrelated (two Windows-only EBUSY teardown unlinks of storage.sqlite, plus three
long-standing assertion failures in combo/cursor-agent/GLM-alias tests). In particular all seven
real cases of model-catalog-cache-swr-8728.test.ts pass — including "an expired successful
entry is served immediately while a refresh runs behind it", which exercises the new default
scheduler's fallback path.

Tests Added Or Updated

None added. The contract tests already existed and were red on purpose; this PR makes the
production code satisfy them without touching them.

Coverage Notes

src/app/api/v1/models/catalogCache.ts and catalog.ts are the production files touched. The
scheduler path is covered by model-catalog-cache-swr-8728.test.ts (default/fallback scheduler)
and v1-models-swr-response-flush-8728.test.ts (injected scheduler + flush ordering).

Reviewer Notes

The open question in the issue — "is route.ts in scope of tsconfig.typecheck-core.json?" —
is answered: no, and that is why the excess argument survived.
That config lists 27 explicit
files in files[] with "include": [], and none is under src/app/api/. tsc does catch the
call: adding the three files to a scratch config extending tsconfig.typecheck-core.json reports

src/app/api/v1/models/route.ts(35,5): error TS2554: Expected 1-2 arguments, but got 3.

on base — and that error is gone on this branch. Comparing the full error sets from that
scratch config: 96 errors on base, 95 here, the only difference being that TS2554. This PR
introduces no new type errors. (The other 95 are pre-existing noise from running strictNullChecks
over files that were never in that config's scope — not something this PR should churn.)

Widening the typecheck scope to cover src/app/api/v1/models/ would have caught this at the
commit that introduced it, and would catch the next one. I have deliberately not done it
here: it surfaces ~95 unrelated errors in these three files alone, so it is its own piece of
work and its own review, not a rider on a bug fix.

Two smaller judgement calls worth a look:

  • The after() fallback is a try/catch, not a feature check. Next exposes no supported way
    to ask "am I in a request scope?", and after() throwing is its documented behaviour outside
    one. The alternative — requiring every non-route caller to inject a scheduler — would turn a
    missing argument into a crash in scripts and tests that legitimately have no response to flush.
  • getStaleWhileRevalidateMs is a function, not a number, so a caller can widen the window
    for one call without this module memoising a value meant to hold once. Only the flush test uses
    it today (() => Number.POSITIVE_INFINITY), to keep the test independent of the 30 s constant.

@ntdat812
ntdat812 requested a review from diegosouzapw as a code owner August 26, 2026 02:21
@ntdat812
ntdat812 force-pushed the fix/models-after-scheduler-11551 branch from 478f98d to a425403 Compare August 26, 2026 02:32
cheaper-inference#11551)

`src/app/api/v1/models/route.ts` passes a third argument to
`getUnifiedModelsResponse`:

    { scheduleBackgroundRefresh: (task) => after(task) }

but the function has accepted two parameters since cheaper-inference#9199 removed the injection
point, so the object was silently discarded. `catalogCache` never imported
`after` at all and kept scheduling the stale-while-revalidate rebuild with
`setTimeout(…, 0)`.

`setTimeout(…, 0)` only yields a turn of the event loop — the rebuild still
runs while the response is in flight, so a catalog build that blocks for
hundreds of milliseconds delays the stale body the client was promised. That
is the flush-before-rebuild guarantee cheaper-inference#8728 was written for, and it was not
in effect.

- `catalogCache` gains an exported `BackgroundRefreshScheduler` and a
  `defaultBackgroundRefreshScheduler` built on `after()`, falling back to a
  macrotask when called outside a Next request scope (unit tests, scripts)
  where `after()` throws and flush ordering is meaningless anyway.
- `resolveCachedCatalogResponse` takes `scheduleBackgroundRefresh` and
  `getStaleWhileRevalidateMs` on its existing options object. Neither
  participates in the cache key.
- `getUnifiedModelsResponse` accepts the third argument the route has been
  passing all along and forwards it.

No behaviour change for callers that inject nothing beyond where the rebuild
runs.

Closes cheaper-inference#11551
@ntdat812

Copy link
Copy Markdown
Contributor Author

CI on this PR is red, and I do not think any of it is mine. Recording the check rather than
leaving it to be re-derived.

The same six gates fail — Docs Gates (fast-path), Fast Quality Gates,
No new ESLint warnings, and three of the four Unit Tests fast-path shards — on every open
pull request against release/v3.8.51 that I can see, including #11577, #11580 and #11582,
which are not mine and touch entirely different files. The failing unit tests
(db-cleanup, db-settings-crud, cursor-renewal, kiro-multi-account-isolation,
openrouter-provider-stats, namespace-tool-identity-integration,
t31-t33-t34-t38-model-specs, authz/route-guard-version-get-exemption) have no overlap with
this diff.

Locally, at the base commit this branch is rebased onto (6eb7bf0ba9), the focused tests for
this change pass and the before/after evidence in the description reproduces.

Happy to rebase again whenever the base is green — it has moved 45 commits since I opened this,
so I would rather rebase once against a known-good tip than keep chasing it. Related: #9282.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Obrigado — mesmo bug e mesma correção de #11574 (terceiro argumento de getUnifiedModelsResponse sendo descartado, scheduler via after()). Vou mergear #11574 (aberta primeiro) e fechar esta para evitar duplicidade.

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.

fix(api): a fiação do after() em /v1/models está morta — refresh SWR nunca usa o scheduler injetado

2 participants