Repository navigation
fix(core): classify models.dev refresh failures - #56752
shreyanshj10 wants to merge 2 commits into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good overall. On the base code, a transport failure produced only the raw Cause(...) line, and an EACCES defect escaped refresh and stopped the periodic refresh loop. With this change both are classified, an identical repeat drops to Debug, and the loop keeps running. I checked this by running the two new tests against the base source (both fail) and with the change (18/18 pass); bun typecheck in packages/core is clean. I couldn't do an end-to-end CLI run here because the local server wouldn't start in this environment, on the base and on this branch alike.
Public API: unchanged. Only a log message and its fields change.
One thing worth fixing: the message always says the bundled catalog is still in use, which is usually not true (see inline).
Related: #41360
| const log = failureKey === lastFailure ? Effect.logDebug : Effect.logError | ||
| lastFailure = failureKey | ||
| return log( | ||
| `Failed to refresh models.dev; the bundled catalog is still in use, retrying in ${Duration.format(ttl)}`, |
There was a problem hiding this comment.
"the bundled catalog is still in use" is often not accurate. After any successful fetch, populate serves the KV-cached catalog (or options.file when set), not the bundled snapshot, and with snapshot: false there may be no bundled catalog at all. Your own transport test starts from a cached catalog. Something neutral like "the current catalog is still in use" would stay true in every case.
Issue for this PR
Closes #56742
Type of change
What does this PR do?
The periodic models.dev refresh logged the raw cause and nothing else, so a transport failure and an
EACCESon the state directory read identically, and neither said what to try or whether the session was affected:It now classifies the failure and logs the class, the endpoint, the remedy, the fallback and the retry interval:
A permission failure instead reads
reason=filesystem detail="EACCES: permission denied, mkdir '/home/user/.local/state/opencode/locks'" next="check ownership and permissions on the opencode state and data directories".Three things worth calling out:
Debug, so the defaultInfothreshold shows one line per distinct failure, and a success clears the key so a failure that comes back is reported again.HttpClientErrorreason tag plusErrorSummary.frominstead of pullingnetworkFailure()out ofaisdk.ts.networkFailurecannot see the network error at this site — anHttpClientErrorcarries it onreason.cause, not on its owncause— and its message allowlist does not contain the shape in the report, whileErrorSummary.fromis already how this repo puts an error chain in a log and reports whatever code it finds. Glad to switch to the shared classifier if you would rather have it; it is a small change either way.EACCEShalf arrives as a defect, andEffect.ignoredoes not catch defects, so it escapedrefreshand took the periodic refresh fiber with it after one line. I recover from it the waywriteCachejust above does, withcatchCauseIfon anything that is not interrupt-only. That is also what makes the retry in the message true for that class.How did you verify your code works?
Two tests in
packages/core/test/models.test.ts, one per class, asserting the level and the fields through a captured logger. Both fail ondev: the transport one captures no classified line at all, and the filesystem one never reaches its assertions because the defect escapesrefresh.From
packages/core:bun run test test/models.test.ts(18 pass) andbun typecheck. Alsocatalog,model,model-resolverandplugin/models-dev(95 pass), andbun run checkat the root.Screenshots / recordings
N/A, log output only.
Checklist
Added by the opencode bot (AI-generated).
Related: #41360