Skip to content

Commit 977e207

Browse files
committed
fix(models): [#1052 D14 review-fix M3] don't crash on non-JSON error body + fire refresh at boot
Two consensus-review findings on the D14 commit (`248eaaf86c`): 1. Pre-existing crash: `ModelsDev.Data()`'s `Flock.withLock` branch calls `JSON.parse(result2.text)` unconditionally after `fetchApi()` returns — even when `result2.ok === false` and the body is an HTML 5xx error page. That throws `SyntaxError` and blocks model initialization. Bug existed before D14 but was rarely exposed because the eager import-time refresh() warmed the disk cache first, so most subsequent `Data()` calls returned via `readJson`. Post-D14 more first-calls fall through to fetch, so the crash window widens. Fix: return `{}` when `!result2.ok` — callers already tolerate an empty catalog (Provider.state produces no models.dev-derived entries, same UX as `OPENCODE_DISABLE_MODELS_FETCH=1`). 2. Reintroduce a boot-time refresh without holding the event loop: 4/6 reviewers flagged the loss of "fresh at boot" as a real regression for short-lived commands. Fix: `Promise.resolve().then(() => refresh().catch())`. A microtask can't itself keep Bun alive, and if the fetch it schedules is still in flight at process-exit the snapshot covers callers on the next run. Preserves the load-bearing part of the D14 fix (no `setTimeout` keeping the loop alive) while restoring near-immediate cache warming. If this ever reintroduces the unshare-net sanity hang on CI, drop the Promise.then line — the snapshot alone keeps release binaries functional offline. Also fixes the misleading comment "always have model metadata even on a cold-start with no network" — accurate for release binaries but false in dev-mode where the snapshot isn't embedded. Reworded to say so. Verified: typecheck clean; `--version` exits in ~1s cleanly; existing D12 phase-label test still passes.
1 parent c237795 commit 977e207

1 file changed

Lines changed: 29 additions & 17 deletions

File tree

‎packages/opencode/src/provider/models.ts‎

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,18 @@ export namespace ModelsDev {
121121
const result = await Filesystem.readJson(Flag.OPENCODE_MODELS_PATH ?? filepath).catch(() => {})
122122
if (result) return result
123123
const result2 = await fetchApi()
124-
if (result2.ok) {
125-
await Filesystem.write(filepath, result2.text).catch((e) => {
126-
log.error("Failed to write models cache", { error: e })
127-
})
128-
}
124+
// altimate_change — #1052 D14 review-fix (M3): fetchApi returning a non-2xx
125+
// (e.g. 5xx with an HTML error body) previously fell through to
126+
// `JSON.parse(<HTML>)` and crashed with SyntaxError. Return an empty
127+
// catalog instead — callers already tolerate empty results (Provider.state
128+
// just yields no models.dev-derived providers, which is the same UX as
129+
// running with OPENCODE_DISABLE_MODELS_FETCH=1). Pre-D14 this rarely
130+
// fired because the eager refresh usually warmed the disk cache; post-D14
131+
// more first-calls fall through to fetch, so more chances to hit the crash.
132+
if (!result2.ok) return {}
133+
await Filesystem.write(filepath, result2.text).catch((e) => {
134+
log.error("Failed to write models cache", { error: e })
135+
})
129136
return JSON.parse(result2.text)
130137
})
131138
})
@@ -164,19 +171,24 @@ if (!Flag.OPENCODE_DISABLE_MODELS_FETCH && !process.argv.includes("--get-yargs-c
164171
//
165172
// Callers that need model data use `ModelsDev.Data()`, which resolves in this
166173
// priority order: (1) local disk cache, (2) bundled snapshot at
167-
// `models-snapshot.ts` (always embedded in release binaries, regenerated at
168-
// each build), (3) `Flock.withLock(...) → fetchApi()` only when both are
169-
// absent. The bundled snapshot means release-binary users always have model
170-
// metadata even on a cold-start with no network. Long-running processes
171-
// (TUI, serve) still receive updates via the hourly `setInterval` below
172-
// (`.unref()`'d so it never blocks exit). Short-lived commands rely on the
173-
// snapshot's release-time freshness.
174+
// `models-snapshot.ts` (embedded in release binaries — regenerated at each
175+
// build; dev-mode builds without the snapshot fall through to fetch), (3)
176+
// `Flock.withLock(...) → fetchApi()` only when both are absent. Release
177+
// binaries therefore have release-time model metadata even on a cold-start
178+
// with no network. Long-running processes (TUI, serve) still receive updates
179+
// via the hourly `setInterval` below (`.unref()`'d so it never blocks exit).
180+
//
181+
// Trade-off: without an eager fetch, models added to models.dev between
182+
// releases would not appear until the hourly interval below fires. The
183+
// fire-and-forget refresh() below narrows that window without holding the
184+
// event loop — a microtask can't itself keep Bun alive, and if the fetch it
185+
// schedules is still in flight at process-exit, the snapshot covers callers
186+
// on the next run. The load-bearing part of the D14 fix (removing the
187+
// setTimeout(...,0)-wrapped fetch that kept the loop alive) is preserved.
174188
//
175-
// Trade-off: models added to models.dev between releases do not appear in
176-
// short-lived commands until the next release rebuild. Bounded by the release
177-
// cadence (~weekly). Users needing bleeding-edge model metadata can run
178-
// `altimate-code auth login` or open the TUI, both of which trigger the
179-
// hourly interval on start-up rebase.
189+
// If this reintroduces the unshare-net hang on CI, drop the Promise.then
190+
// line — the snapshot alone still keeps release binaries functional offline.
191+
Promise.resolve().then(() => ModelsDev.refresh().catch(() => {}))
180192
setInterval(async () => {
181193
await ModelsDev.refresh()
182194
}, 60 * 60 * 1000).unref()

0 commit comments

Comments
 (0)