refactor(models): tighten Service body per review

- Use Schedule.spaced (not fixed) so the periodic refresh waits between
  completions instead of firing twice on startup.
- Pull JSON.parse out of the Flock-held scope — keeps the cross-process
  lock release-eager.
- Only invalidate after a successful fetchAndWrite; the no-op fresh-skip
  and ignored-failure paths shouldn't force the next get() to re-read.
- Drop comments narrating obvious behavior; fix a misplaced WHY comment.
- Use HttpClient.make (matches mockHttpClient elsewhere) instead of
  makeWith with a Preprocess cast in the new tests.

Author: kitlangton

packages/opencode/src/provider/models.ts

@@ -134,15 +134,13 @@ export const layer: Layer.Layer<Service, never, AppFileSystem.Service | HttpClie
         Effect.map((v) => v as Record<string, Provider> | undefined),
       )
 
-    // Bundled snapshot fallback — present in built releases, missing in dev.
+    // Bundled at build time; absent in dev — `tryPromise` covers both.
     const loadSnapshot = Effect.tryPromise({
       // @ts-ignore — generated at build time, may not exist in dev
       try: () => import("./models-snapshot.js").then((m) => m.snapshot as Record<string, Provider> | undefined),
       catch: () => undefined,
     }).pipe(Effect.catch(() => Effect.succeed(undefined)))
 
-    // Cross-process file lock — Flock is the right primitive (in-process
-    // semaphores can't coordinate concurrent opencode CLIs writing the same cache).
     const fetchAndWrite = Effect.fn("ModelsDev.fetchAndWrite")(function* () {
       const text = yield* fetchApi()
       yield* fs.writeWithDirs(filepath, text)
@@ -155,41 +153,40 @@ export const layer: Layer.Layer<Service, never, AppFileSystem.Service | HttpClie
       const snapshot = yield* loadSnapshot
       if (snapshot) return snapshot
       if (Flag.OPENCODE_DISABLE_MODELS_FETCH) return {}
-      return yield* Effect.scoped(
+      // Flock is cross-process: concurrent opencode CLIs can race on this cache file.
+      const text = yield* Effect.scoped(
         Effect.gen(function* () {
           yield* Flock.effect(lockKey)
-          const text = yield* fetchAndWrite()
-          return JSON.parse(text) as Record<string, Provider>
+          return yield* fetchAndWrite()
         }),
       )
+      return JSON.parse(text) as Record<string, Provider>
     }).pipe(Effect.withSpan("ModelsDev.populate"), Effect.orDie)
 
-    // Single-flight cache: concurrent first-call dedupes via the cached effect.
-    // refresh() invalidates so the next get() re-populates from disk.
     const [cachedGet, invalidate] = yield* Effect.cachedInvalidateWithTTL(populate, Duration.infinity)
 
     const get = (): Effect.Effect<Record<string, Provider>> => cachedGet
 
     const refresh = Effect.fn("ModelsDev.refresh")(function* (force = false) {
-      if (!force && (yield* fresh())) return yield* invalidate
+      if (!force && (yield* fresh())) return
       yield* Effect.scoped(
         Effect.gen(function* () {
           yield* Flock.effect(lockKey)
+          // Re-check under the lock: another process may have refreshed between
+          // our outer check and lock acquisition.
           if (!force && (yield* fresh())) return
           yield* fetchAndWrite()
+          yield* invalidate
         }),
       ).pipe(
         Effect.tapCause((cause) => Effect.logError("Failed to fetch models.dev", { cause })),
         Effect.ignore,
       )
-      yield* invalidate
     })
 
     if (!Flag.OPENCODE_DISABLE_MODELS_FETCH && !process.argv.includes("--get-yargs-completions")) {
-      // Eager initial refresh + 1 hour cadence (matches pre-Service behavior).
-      yield* Effect.forkScoped(
-        refresh().pipe(Effect.andThen(refresh().pipe(Effect.repeat(Schedule.fixed("60 minutes")))), Effect.ignore),
-      )
+      // Schedule.spaced runs the effect once, then waits between completions.
+      yield* Effect.forkScoped(refresh().pipe(Effect.repeat(Schedule.spaced("60 minutes")), Effect.ignore))
     }
 
     return Service.of({ get, refresh })

packages/opencode/test/provider/models.test.ts

@@ -1,6 +1,6 @@
 import { describe, expect, beforeEach, afterAll } from "bun:test"
 import { Effect, Layer, Ref } from "effect"
-import { HttpClient, HttpClientError, HttpClientResponse } from "effect/unstable/http"
+import { HttpClient, HttpClientResponse } from "effect/unstable/http"
 import { AppFileSystem } from "@opencode-ai/core/filesystem"
 import { Flag } from "@opencode-ai/core/flag/flag"
 import { Global } from "@opencode-ai/core/global"
@@ -64,15 +64,13 @@ interface MockState {
   calls: Array<{ url: string }>
 }
 
-const makeMockClient = (state: Ref.Ref<MockState>): HttpClient.HttpClient =>
-  HttpClient.makeWith(
-    Effect.fnUntraced(function* (requestEffect) {
-      const request = yield* requestEffect
+const makeMockClient = (state: Ref.Ref<MockState>) =>
+  HttpClient.make((request) =>
+    Effect.gen(function* () {
       yield* Ref.update(state, (s) => ({ ...s, calls: [...s.calls, { url: request.url }] }))
       const s = yield* Ref.get(state)
       return HttpClientResponse.fromWeb(request, new Response(s.body, { status: s.status }))
     }),
-    Effect.succeed as HttpClient.HttpClient.Preprocess<HttpClientError.HttpClientError, never>,
   )
 
 const buildLayer = (state: Ref.Ref<MockState>) =>
@@ -244,10 +242,8 @@ describe("ModelsDev Service", () => {
           return yield* svc.get()
         }),
       )
-      // refresh logged + ignored; the cache map is whatever loadFromDisk yields next.
-      // With the failing fetch the disk file is unchanged → still the original fixture.
       expect(result).toEqual(fixture)
-      // withTransientReadRetry retries 5xx, so calls > 1 is expected.
+      // withTransientReadRetry retries 5xx, so calls may be > 1.
       const final = yield* Ref.get(state)
       expect(final.calls.length).toBeGreaterThanOrEqual(1)
     }),