Repository navigation
fix(core): avoid holding models.dev refresh lock across network timeout - #61120
Bearmancer wants to merge 3 commits into
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for looking into startup latency. This doesn't look mergeable as it stands.
How I checked: I wrote a throwaway test using the real ModelsDev layer, with a mock models.dev that takes 3s to answer. With the default bundled snapshot, get() returned the full catalog in about 330 ms on both the base and this PR. populate() returns the file, the cached copy or the bundled snapshot before it ever takes lock. It only takes the lock when none of those exist, which in practice means snapshot: false with an empty cache. In that case it fetches anyway, so it would hit the same 10s timeout. So a slow refresh doesn't block foreground reads on the base. If you're seeing the 503s from #61118, the wait is probably somewhere else, and a trace or a reproduction would help find it.
The change also causes two problems (see inline comments):
- Type check fails:
bun run typecheck:tscinpackages/corefails withTS2322.refreshis nowEffect<void, SchemaError>, but the service declaresEffect<void>. - Refresh errors are no longer swallowed: before, the whole refresh was wrapped in
tapCause(logError)+Effect.ignore; now onlyfetchApiis. A response that fails to decode now makesrefresh(true)fail (I checked this with an invalid body). In the backgroundrefresh().pipe(Effect.repeat(...), Effect.ignore)loop, that failure ends the repeat, so the catalog would stop refreshing until restart.
Public API: unchanged.
| return | ||
| } | ||
| if (!response) return | ||
| yield* lock.withPermit( |
There was a problem hiding this comment.
Before, the outer tapCause(logError) + Effect.ignore covered everything in refresh. Now decodeCatalog (and the KV reads and writeMeta above) can fail out of refresh. That gives refresh a SchemaError error type, which fails tsc against readonly refresh: (force?: boolean) => Effect.Effect<void> (line 693). In the background loop refresh().pipe(Effect.repeat(Schedule.spaced(ttl)), Effect.ignore), one bad response would end the repeat and stop periodic refreshes for the rest of the process.
| @@ -658,33 +658,31 @@ export const layer = (options?: Options) => | |||
| const get = (): Effect.Effect<readonly Snapshot[]> => cachedGet | |||
|
|
|||
| const refresh = Effect.fn("ModelsDev.refresh")(function* (force = false) { | |||
There was a problem hiding this comment.
With the default config, populate() returns the cached copy or the bundled snapshot before it takes lock. It only takes the lock when there is no file, no cache and no snapshot, and in that case it has to fetch too. So moving fetchApi out of the lock doesn't unblock get() in the scenario from #61118. With a 3s mock fetch, get() returned in ~330 ms on the base as well. Moving the TTL/ETag check and writeMeta out of the lock also lets a forced refresh and the periodic one run at the same time and overwrite each other's metadata.
|
⁰
…On Mon, Nov 2, 2026, 3:14 AM opencodejr[bot] ***@***.***> wrote:
***@***.***[bot]* commented on this pull request.
Thanks for looking into startup latency. This doesn't look mergeable as it
stands.
How I checked: I wrote a throwaway test using the real ModelsDev layer,
with a mock models.dev that takes 3s to answer. With the default bundled
snapshot, get() returned the full catalog in about 330 ms on both the
base and this PR. populate() returns the file, the cached copy or the
bundled snapshot before it ever takes lock. It only takes the lock when
none of those exist, which in practice means snapshot: false with an
empty cache. In that case it fetches anyway, so it would hit the same 10s
timeout. So a slow refresh doesn't block foreground reads on the base. If
you're seeing the 503s from #61118
<#61118>, the wait is
probably somewhere else, and a trace or a reproduction would help find it.
The change also causes two problems (see inline comments):
- *Type check fails:* bun run typecheck:tsc in packages/core fails
with TS2322. refresh is now Effect<void, SchemaError>, but the service
declares Effect<void>.
- *Refresh errors are no longer swallowed:* before, the whole refresh
was wrapped in tapCause(logError) + Effect.ignore; now only fetchApi
is. A response that fails to decode now makes refresh(true) fail (I
checked this with an invalid body). In the background refresh().pipe(Effect.repeat(...),
Effect.ignore) loop, that failure ends the repeat, so the catalog
would stop refreshing until restart.
Public API: unchanged.
Related: #60854 <#60854>,
#56119 <#56119>
------------------------------
In packages/core/src/models-dev.ts
<#61120 (comment)>:
> + )
+ if (!force && stored && Date.now() - stored.updatedAt < Duration.toMillis(ttl)) return
+ const response = yield* fetchApi(force ? undefined : stored?.etag).pipe(
+ Effect.tapCause((cause) => Effect.logError("Failed to fetch models.dev", { cause })),
+ Effect.orElseSucceed(() => undefined),
+ )
+ const digest = response && bodyDigest(response.text)
+ // models.dev rarely changes between polls; skip invalidation and
+ // the Refreshed event for a byte-identical body so downstream
+ // provider/model update listeners stay quiet.
+ if (!force && stored && (!response || stored.digest === digest)) {
+ yield* writeMeta(Date.now(), stored.digest, response?.etag ?? stored.etag)
+ return
+ }
+ if (!response) return
+ yield* lock.withPermit(
Before, the outer tapCause(logError) + Effect.ignore covered everything
in refresh. Now decodeCatalog (and the KV reads and writeMeta above) can
fail out of refresh. That gives refresh a SchemaError error type, which
fails tsc against readonly refresh: (force?: boolean) =>
Effect.Effect<void> (line 693). In the background loop refresh().pipe(Effect.repeat(Schedule.spaced(ttl)),
Effect.ignore), one bad response would end the repeat and stop periodic
refreshes for the rest of the process.
------------------------------
In packages/core/src/models-dev.ts
<#61120 (comment)>:
> @@ -658,33 +658,31 @@ export const layer = (options?: Options) =>
const get = (): Effect.Effect<readonly Snapshot[]> => cachedGet
const refresh = Effect.fn("ModelsDev.refresh")(function* (force = false) {
With the default config, populate() returns the cached copy or the
bundled snapshot before it takes lock. It only takes the lock when there
is no file, no cache and no snapshot, and in that case it has to fetch too.
So moving fetchApi out of the lock doesn't unblock get() in the scenario
from #61118 <#61118>. With a
3s mock fetch, get() returned in ~330 ms on the base as well. Moving the
TTL/ETag check and writeMeta out of the lock also lets a forced refresh
and the periodic one run at the same time and overwrite each other's
metadata.
—
Reply to this email directly, view it on GitHub
<#61120?email_source=notifications&email_token=BVRSBAATKUWTO7L7A7R3TIT5W7GXBA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKNZTGA4TIMBRGM42M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#pullrequestreview-5730940139>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BVRSBAFP44P6UO7QBZV4Q435W7GXBAVCNFSNUABFKJSXA33TNF2G64TZHM4TONJXGM2DGMJZHNEXG43VMU5TMMJVGM3TMNJTG4ZKC5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BVRSBADSYX5T6MPOWBEEGQT5W7GXBA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKNZTGA4TIMBRGM42M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSVGM33PORSXEX3JN5ZQ>
and Android
<https://github.com/notifications/mobile/android/BVRSBACAZ45H73ZTJEK7HHL5W7GXBA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKNZTGA4TIMBRGM42M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSXGM33PORSXEX3BNZSHE33JMQ>.
Download it today!
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
There was a problem hiding this comment.
The new commit only merges dev. packages/core/src/models-dev.ts hasn't changed, so both earlier points are still open:
- Type check:
bun run typecheck:tscstill fails withTS2322onrefresh. - Error handling: errors from
refreshare no longer swallowed, and one bad response can end the periodic refresh loop. - Unreproduced bug: the hang described in #61118 doesn't happen, because
populate()returns the cache or the bundled snapshot before it takes the lock.
Public API: unchanged.
|
@opencodejr — Thanks for the careful review and catching those issues! Addressed both in commit 0da299d:
|
There was a problem hiding this comment.
The new commit fixes the error-handling regression: the whole refresh is wrapped in tapCause(logError) + Effect.ignore again, and bun run typecheck:tsc in packages/core now passes. bun test test/models.test.ts passes, and a refresh with a body that isn't valid JSON is swallowed as before.
The main concern from the earlier review is still open: with the default bundled snapshot, get() doesn't wait on lock. With a 3s mock fetch it returned in ~335 ms on the base too. So I still can't see a user-visible problem this change fixes.
One new side effect: the lock is now released during the fetch. Two refreshes that start close together, such as a forced one and the periodic one, now both fetch models.dev. Before, the second waited, saw the fresh TTL and skipped the request. Two forced refreshes can also both publish Refreshed. This is minor, but it's a cost without a shown benefit.
Public API: unchanged.
There was a problem hiding this comment.
The new refreshLock addresses the side effect from my last review. Refreshes are serialized again, so a second refresh waits, sees the fresh TTL and skips the fetch. Tests (test/models.test.ts) and bun run typecheck:tsc pass.
The main point is still open: with the bundled snapshot or a cached catalog, get() never waits on lock. I measured ~240 ms with a 3s mock fetch, the same as the base. The PR now uses two semaphores to end up behaving like the base, apart from the no-snapshot, no-cache path, where populate() fetches on its own anyway. Without a reproduction showing get() blocked on the refresh lock, I don't think this extra locking is worth merging.
Public API: unchanged.
Verification (owner triage, 2026-11-08)
No code change needed from triage; the refresh error-handling shape holds against the declared Tracking: GitHub-sweep backlog #1 lane omo#10704 (memory-core) closed today; this lane verified green locally. |
Signed-off-by: Bearmancer <[email protected]>
…ock in ModelsDev.refresh
a22b38e to
96b2c49
Compare
|
Rebased onto origin/dev: |
There was a problem hiding this comment.
This push only moves the branch onto newer dev. The change to models-dev.ts is the same as at my last review, and tests and tsc still pass on the new base. My open point about the change not unblocking get() in the default setup still applies.
Public API: unchanged.
Bearmancer
left a comment
Bearmancer
left a comment
There was a problem hiding this comment.
Verified against base dev (packages/core/src/models-dev.ts): get() returns the memoized cachedGet with infinite TTL, so after the first call it never touches lock. populate() resolves file, then cache, then the bundled snapshot before ever taking lock, and the default setup always has the snapshot. Base refresh() holds lock across the fetch, but get() does not wait on it. So the lock-split changes nothing observable on the default path, matching the ~240ms base-vs-PR measurements.
Agreed on the substance: without a reproduction showing get() blocked on the refresh lock, this adds locking without a user-visible effect.
One candidate path I can see: snapshot: false with an empty cache, where first populate() takes lock and fetches. Is that the path behind the 503s in #61118? A trace or repro pointing at it would let this PR target the actual block.
|
Closing this. The bot's design-objection reviews (5755878343, 5856486904) point out that with the bundled snapshot or a cached catalog,
|
Issue for this PR
Closes #61118
Type of change
What does this PR do?
In
packages/core/src/models-dev.ts,refresh()previously acquiredlock.withPermitbefore awaitingfetchApi(), which has a 10-second timeout. Ifmodels.devwas slow, firewalled, or unreachable during boot,lockwas held for up to 10 seconds. This prevented foreground calls toModelsDev.get()(which invokepopulate()and require the same lock) from reading from local cache or the bundled snapshot, causing/api/modeland/api/agentto return HTTP 503 and stalling client startup.This PR moves
fetchApi()outsidelock.withPermitinrefresh(). The lock is now acquired only after the HTTP response arrives when decoding and updating the cache and publishingModelsDev.Event.Refreshed. If the background network fetch fails or times out, foreground catalog reads are never blocked.How did you verify your code works?
Verified that
ModelsDev.refresh()executesfetchApi()outsidelock.withPermit, allowing concurrent calls topopulate()to acquirelockand return cached/snapshot catalog data immediately without waiting on network timeouts.Visual
None (non-visual core engine fix).
Screenshots / recordings
No UI changes.
Checklist
Added by the opencode bot (AI-generated).
Related: #60854, #56119